-
Notifications
You must be signed in to change notification settings - Fork 907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable #4159
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4159 +/- ##
==========================================
- Coverage 90.76% 90.73% -0.03%
==========================================
Files 215 215
Lines 39497 39497
==========================================
- Hits 35848 35839 -9
- Misses 3649 3658 +9
Continue to review full report at Codecov.
|
3c1aac9
to
4a518f1
Compare
tsl/src/compression/create.c
Outdated
{ | ||
Oid compress_relid = compress_ht->main_table_relid; | ||
ColumnDef *coldef; | ||
AlterTableCmd *addcol_cmd; | ||
HeapTuple attTuple; | ||
int attnum; | ||
bool skipped = false; /* postgres operates on its table first, so the column may have been |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't really assume this here. Maybe this function is going to be called from a different place in the future. There is no guarantee that postgres operated on its table first.
tsl/src/compression/create.c
Outdated
@@ -1057,28 +1087,31 @@ tsl_process_compress_table(AlterTableCmd *cmd, Hypertable *ht, | |||
* This function specifically adds the column to the internal compression table. | |||
*/ | |||
void | |||
tsl_process_compress_table_add_column(Hypertable *ht, ColumnDef *orig_def) | |||
tsl_process_compress_table_add_column(Hypertable *ht, const AlterTableCmd *cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want to tie this to an ALTER TABLE command? We should be able to add a column on demand if we like. I don't see what missing_ok
offers us here. In any case, if we really needed missing_ok
we could add an extra bool
argument.
In this case if missing_ok
is true, then it is a no-op that prints a warning, otherwise it's an exception. Is this the idea here?
6850ee8
to
53d3855
Compare
tsl/src/compression/create.c
Outdated
else | ||
{ | ||
ReleaseSysCache(attTuple); | ||
} | ||
|
||
/* alter the table and add column */ | ||
AlterTableInternal(compress_relid, list_make1(addcol_cmd), true); | ||
modify_compressed_toast_table_storage(compress_cols, compress_relid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be conditional -> do it only if a new column is added
tsl/src/compression/create.c
Outdated
Assert(TS_HYPERTABLE_HAS_COMPRESSION_ENABLED(ht)); | ||
} | ||
/* add catalog entries for the new column for the hypertable */ | ||
compresscolinfo_add_catalog_entries(&compress_cols, orig_htid); | ||
if (new_column || no_compression_table) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add catalog entries only if this is a new column, otherwise you will have duplicates.
if (new_column || no_compression_table) | |
if (new_column ) |
tsl/src/compression/create.c
Outdated
/* the column doesn't exist yet */ | ||
new_column = true; | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is Release conditional? Shouldn't this be released before leaving the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not needed when the search fails.
tsl/src/compression/create.c
Outdated
} | ||
else | ||
{ | ||
no_compression_table = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this variable is not needed.
tsl/src/compression/create.c
Outdated
if (!HeapTupleIsValid(attTuple)) | ||
{ | ||
/* the column doesn't exist yet */ | ||
new_column = true; | ||
} | ||
else | ||
{ | ||
ReleaseSysCache(attTuple); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!HeapTupleIsValid(attTuple)) | |
{ | |
/* the column doesn't exist yet */ | |
new_column = true; | |
} | |
else | |
{ | |
ReleaseSysCache(attTuple); | |
} | |
/* the column doesn't exist yet */ | |
new_column = !HeapTupleIsValid(attTuple); | |
ReleaseSysCache(attTuple); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Patch looks good, but a few items that I think might be good to add a test for a compressed distributed table and make sure that it behaves in a sane manner.
The most sensible place to call the function to add a compressed table column is in process_altertable_end_subcmd
, which is called after the main statement has executed. At this point you know that adding the column succeeded and that all that remains is to add the column to the compressed table. At this point is is not necessary to do any checking if the column exists and you can just proceed with executing the AlterTableStmt
with the same options as before, but change the table name.
NOTICE: column "medium" of relation "metric" already exists, skipping | ||
NOTICE: column "medium" of relation "_compressed_hypertable_24" already exists, skipping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Is it possible to silence the second one? This uses a name for an internal tables and not particularly useful to the user. Feel free to skip this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's possible because it is a internal postgres message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be possible to set the missing_ok
to false
and then use PG_TRY
and PG_CATCH
to eliminate the message. Not critical though, so we could do it in a separate PR.
ALTER TABLE metric ADD COLUMN IF NOT EXISTS "medium" VARCHAR ; | ||
-- also add one without IF NOT EXISTS | ||
ALTER TABLE metric ADD COLUMN "medium_1" VARCHAR ; | ||
ALTER TABLE metric ADD COLUMN "medium_1" VARCHAR ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALTER TABLE metric ADD COLUMN "medium_1" VARCHAR ; | |
\set ON_ERROR_STOP 0 | |
ALTER TABLE metric ADD COLUMN "medium_1" VARCHAR ; | |
\set ON_ERROR_STOP 1 |
tsl/src/compression/create.c
Outdated
/* the column doesn't exist yet */ | ||
new_column = true; | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not needed when the search fails.
This might require some rewrite of the |
@konskov looking more deeply for this PR I think (IMHO) is better to passdown the |
6f3bf88
to
e663fb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor change requested. Changes look good.
@@ -1067,6 +1067,14 @@ tsl_process_compress_table_add_column(Hypertable *ht, ColumnDef *orig_def) | |||
|
|||
coloid = LookupTypeNameOid(NULL, orig_typname, false); | |||
compresscolinfo_init_singlecolumn(&compress_cols, colname, coloid); | |||
|
|||
FormData_hypertable_compression *ht_comp = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move the newly added lines to line 1067.
So the check would be the first thing in the function (before any initialization etc) .
FROM generate_series('2021-07-01 00:00:00'::timestamp, | ||
'2021-08-17 00:02:00'::timestamp, '30 s'::interval) s; | ||
|
||
SELECT compress_chunk(show_chunks('metric')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should rewrite the query with order by (so that test output is consistent)
SELECT compress_chunk(ch) FROM show_chunks('metric') ch ORDER BY 1;
e663fb1
to
aed4b24
Compare
tsl/src/compression/create.c
Outdated
if (ht_comp) | ||
{ | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (ht_comp) | |
{ | |
return; | |
} | |
if (ht_comp) | |
return; |
I'm pretty sure the format check will pass if you simplify it this way.
aed4b24
to
d21277a
Compare
Stop throwing exception with message "column of relation already exists" when running the command ALTER TABLE ... ADD COLUMN IF NOT EXISTS ... on compressed hypertables. Fix timescale#4087
d21277a
to
8c7765b
Compare
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * #3974 Fix remote EXPLAIN with parameterized queries * #4122 Fix segfault on INSERT into distributed hypertable * #4142 Ignore invalid relid when deleting hypertable * #4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * #4161 Fix memory handling during scans * #4186 Fix owner change for distributed hypertable * #4192 Abort sessions after extension reload * #4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4176 Fix remote EXPLAIN with parameterized queries * timescale#4181 Fix spelling errors and omissions * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes * timescale#4199 Remove signal-unsafe calls from signal handlers * timescale#4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4176 Fix remote EXPLAIN with parameterized queries * timescale#4181 Fix spelling errors and omissions * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes * timescale#4199 Remove signal-unsafe calls from signal handlers * timescale#4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4176 Fix remote EXPLAIN with parameterized queries * timescale#4181 Fix spelling errors and omissions * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes * timescale#4199 Remove signal-unsafe calls from signal handlers * timescale#4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4176 Fix remote EXPLAIN with parameterized queries * timescale#4181 Fix spelling errors and omissions * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes * timescale#4199 Remove signal-unsafe calls from signal handlers * timescale#4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is a patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4176 Fix remote EXPLAIN with parameterized queries * timescale#4181 Fix spelling errors and omissions * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes * timescale#4199 Remove signal-unsafe calls from signal handlers * timescale#4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
This release is a patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * #4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * #4122 Fix segfault on INSERT into distributed hypertable * #4142 Ignore invalid relid when deleting hypertable * #4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * #4161 Fix memory handling during scans * #4176 Fix remote EXPLAIN with parameterized queries * #4181 Fix spelling errors and omissions * #4186 Fix owner change for distributed hypertable * #4192 Abort sessions after extension reload * #4193 Fix relcache callback handling causing crashes * #4199 Remove signal-unsafe calls from signal handlers * #4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain
When an ALTER TABLE ... ADD COLUMN IF NOT EXISTS ... command is executed
and the column already exists, we get an error message that the column already
exists on the compressed hypertable, even though IF NOT EXISTS is specified.
This PR removes this error.
Fix #4087