-
Notifications
You must be signed in to change notification settings - Fork 746
PG18: syntax & semantics behavior in Citus, part 1. #8335
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
Conversation
f8bc384 to
8407c7a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8335 +/- ##
==========================================
+ Coverage 88.92% 88.95% +0.02%
==========================================
Files 287 287
Lines 63151 63152 +1
Branches 7941 7941
==========================================
+ Hits 56156 56175 +19
+ Misses 4681 4668 -13
+ Partials 2314 2309 -5 🚀 New features to boost your workflow:
|
8407c7a to
2f918f1
Compare
| -- (The rangetypes regression test uses the same trick.) | ||
| id int4range, | ||
| valid_at daterange, | ||
| CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS) |
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 think this would only work in create time. i.e. not able to add such a primary key after creating the table, so this should probably be handled separately
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.
Actually add such a primary key after creating the table does work after the table is distributed; I just added tests for the same. So adding a primary key:
ALTER TABLE temporal_rng
ADD CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS);
is propagated to the table's shads:
DETAIL: on server citus@localhost:9702 connectionId: 13
NOTICE: issuing SELECT worker_apply_shard_ddl_command (104206, 'citus', 'ALTER TABLE temporal_rng ADD
CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
;')
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.
Great then. It seems its part of the column name.
/*
* AppendColumnNameList converts a list of columns into comma separated string format
* (colname_1, colname_2, .., colname_n).
*/
void
AppendColumnNameList(StringInfo buf, List *columns)
{
appendStringInfoString(buf, " (");
ListCell *lc;
bool firstkey = true;
foreach(lc, columns)
{
if (firstkey == false)
{
appendStringInfoString(buf, ", ");
}
appendStringInfo(buf, "%s", quote_identifier(strVal(lfirst(lc))));
firstkey = false;
}
appendStringInfoString(buf, " )");
}I also tested distributing the table after adding a constraint, and also adding a new node, and it seems to propagate fine. Could you maybe include one of those tests here, just to be sure. Maybe include the one you commented here.
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.
Sorry, just saw that you already added the tests!!!
| INSERT INTO temporal_rng (id, valid_at) VALUES ('[3,4)', 'empty'); | ||
| ERROR: empty WITHOUT OVERLAPS value found in column "valid_at" in relation "temporal_rng_4754012" | ||
| CONTEXT: while executing command on localhost:xxxxx | ||
| SELECT * FROM temporal_rng ORDER BY id, valid_at; |
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.
maybe add order by
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.
There is an ORDER BY on both columns of the table - is that what you mean?
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.
sorry I added the comment at wrong line. There are some select queries with no order by.
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.
Got it, added.
1463668 to
4a8df04
Compare
naisila
left a comment
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.
LGTM, thanks a lot for the extensive testing. Great to see Citus's current infrastructure seamlessly propagating all these new cool PG18 features.
| -- | ||
| -- Add foreign key constraint with PERIOD clause | ||
| -- This is propagated to worker shards | ||
| ALTER TABLE temporal_fk_rng2rng |
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.
Tested citus_add_node, propagates fine
4a8df04 to
1aaacb4
Compare
src/test/regress/expected/pg18.out
Outdated
| xxx@foo.com | colm@planet.com | ||
| (1 row) | ||
|
|
||
| SELECT * FROM users WHERE id = 1; |
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.
maybe add order by
1aaacb4 to
ede0222
Compare
e302318 to
42d95c8
Compare
Includes PG18 tests for: - OLD/NEW support in RETURNING clause of DML queries (PG commit 80feb727c) - WITHOUT OVERLAPS in PRIMARY KEY and UNIQUE constraints (PG commit fc0438b4e) - COLUMNS clause in JSON_TABLE (PG commit bb766cd) - Foreign tables created with LIKE <table> clause (PG commit 302cf1575) - Foreign Key constraint with PERIOD clause (PG commit 89f908a6d) - COPY command REJECT_LIMIT option (PG commit 4ac2a9bec) - COPY TABLE TO on a materialized view (PG commit 534874fac)
ede0222 to
eba6153
Compare
PG18: syntax & semantics behavior in Citus, part 1.
Includes PG18 tests for:
Partially addresses issue #8250