As of this commit, after recovering the remote transactions, now we release the lock
on pg_dist_transaction while closing it to avoid deadlocks that might occur because
of trying to acquire a lock on pg_dist_authinfo while holding a lock on
pg_dist_transaction. Such a scenario can only cause a deadlock if another transaction
is trying to acquire a strong lock on pg_dist_transaction while holding a lock on
pg_dist_authinfo. As of today, we (implicitly) acquire a strong lock on
pg_dist_transaction only when upgrading Citus to 11.3-1 and this happens when creating
a REPLICA IDENTITY on pg_dist_transaction.
And regardless of the code-path we are in, it should be okay to release the lock there
because all we do after that point is to abort the prepared transactions that are not
part of an in-progress distributed transaction and releasing the lock before doing so
should be just fine.
This also changes the blocking behavior between citus_create_restore_point and the
transaction recovery code-path in the sense that now citus_create_restore_point doesn't
until transaction recovery completes aborting the prepared transactions that are not
part of an in-progress distributed transaction. However, this should be fine because
even before this was possible, e.g., if transaction recovery fails to open a remote
connection to a node.
This pull request addresses Issue #7846, where specific MERGE queries on
non-distributed and distributed tables can result in crashes in certain
scenarios. The issue stems from the usage of `pg_class` catalog table,
and the `FilterShardsFromPgclass` function in Citus. This function goes
through the query's jointree to hide the shards. However, in PG17,
MERGE's join quals are in a separate structure called
`mergeJoinCondition`. Therefore FilterShardsFromPgclass was not
filtering correctly in a `MERGE` command that involves `pg_class`. To
fix the issue, we handle `mergeJoinCondition` separately in PG17.
Relevant PG commit:
0294df2f1f
**Non-Distributed Tables:**
A MERGE query involving a non-distributed table using
`pg_catalog.pg_class` as the source may execute successfully but needs
testing to ensure stability.
**Distributed Tables:**
Performing a MERGE on a distributed table using `pg_catalog.pg_class` as
the source raises an error:
`ERROR: MERGE INTO a distributed table from Postgres table is not yet
supported`
However, in some cases, this can lead to a server crash if the
unsupported operation is not properly handled.
This is the test output from the same test conducted prior to the code
changes being implemented.
```
-- Issue #7846: Test crash scenarios with MERGE on non-distributed and distributed tables
-- Step 1: Connect to a worker node to verify shard visibility
\c postgresql://postgres@localhost::worker_1_port/regression?application_name=psql
SET search_path TO pg17;
-- Step 2: Create and test a non-distributed table
CREATE TABLE non_dist_table_12345 (id INTEGER);
-- Test MERGE on the non-distributed table
MERGE INTO non_dist_table_12345 AS target_0
USING pg_catalog.pg_class AS ref_0
ON target_0.id = ref_0.relpages
WHEN NOT MATCHED THEN DO NOTHING;
SSL SYSCALL error: EOF detected
connection to server was lost
```
Regress test tdigest_aggregate_support has been failing since at least
Citus 12.0, when tdigest extension is installed in Postgres. This
appears to be because of an omission by commit 03832f3 and a change in
the implementation of Postgres random() function (pg commit
[d4f109e4a](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d4f109e4a)).
To reproduce the test diff:
- Checkout [tdigest ](https://github.com/tvondra/tdigest)and run `make;
make install`
- In citus regress directory run `make check-multi` or
`./citus_tests/run_test.py tdigest_aggregate_support`
There are two parts to this commit:
1. Revert `Output: xxxxx` in EXPLAIN VERBOSE. Citus commit fe4ac51
normalized EXPLAIN VERBOSE output because of a change between pg12 and
pg13. When pg12 support was no longer required, the rule was removed
from normalize.sed and `Output: xxxx` was reverted in the impacted
regress output files (03832f3), but `tdigest_aggregate_support` was
omitted.
2. Adjust the query results; the tdigest_aggregate_support test file has
a comment _verifying results - should be stable due to seed while
inserting the data, if failure due to data these queries could be
removed or check for certain ranges_ but the result values in this
commit are consistent across citus 12.0 (pg 15), citus 12.1 (pg 16) and
citus 13.0 (pg 17), or since the Postgres changed their [implementation
of
random](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d4f109e4a),
so proposing to go with these results.
DESCRIPTION: Propagates MERGE ... WHEN NOT MATCHED BY SOURCE
It seems like there is not much needed to be done here.
`get_merge_query_def` from `ruleutils_17` is updated with "WHEN NOT
MATCHED BY SOURCE" therefore `deparse_shard_query` parses the merge
query for execution on the shard correctly.
Relevant PG commit:
https://github.com/postgres/postgres/commit/0294df2f1
DESCRIPTION: Propagates MEMORY and SERIALIZE options of EXPLAIN
The options for `MEMORY` can be true or false. Default is false.
The options for `SERIALIZE` can be none, text or binary. Default is
none.
I referred to how we added support for WAL option in this PR [Support
EXPLAIN(ANALYZE, WAL)](https://github.com/citusdata/citus/pull/4196).
For the tests however, I used the same tests as Postgres, not like the
tests in the WAL PR. I used exactly the same tests as Postgres does, I
simply distributed the table beforehand. See below the relevant Postgres
commits from where you can see the tests added as well:
- [Add EXPLAIN
(MEMORY)](https://github.com/postgres/postgres/commit/5de890e36)
- [Invent SERIALIZE option for
EXPLAIN.](https://github.com/postgres/postgres/commit/06286709e)
This PR required a lot of copying of Postgres static functions regarding
how `EXPLAIN` works for `MEMORY` and `SERIALIZE` options. Specifically,
these copy-pastes were required for updating `ExplainWorkerPlan()`
function, which is in fact based on postgres' `ExplainOnePlan()`:
```C
/* copied from explain.c to update ExplainWorkerPlan() in citus according to ExplainOnePlan() in postgres */
#define BYTES_TO_KILOBYTES(b)
typedef struct SerializeMetrics
static bool peek_buffer_usage(ExplainState *es, const BufferUsage *usage);
static void show_buffer_usage(ExplainState *es, const BufferUsage *usage);
static void show_memory_counters(ExplainState *es, const MemoryContextCounters *mem_counters);
static void ExplainIndentText(ExplainState *es);
static void ExplainPrintSerialize(ExplainState *es, SerializeMetrics *metrics);
static SerializeMetrics GetSerializationMetrics(DestReceiver *dest);
```
_Note_: it looks like we were missing some `buffers` option details as
well. I put them together with the memory option, like the code in
Postgres explain.c, as I didn't want to change the copied code. However,
I tested locally and there is no big deal in previous Citus versions,
and you can also see that existing Citus tests with `buffers true`
didn't change. Therefore, I prefer not to backport "buffers" changes to
previous versions.
This PR adds regression tests to verify REINDEX support with event
triggers. Tests validates trigger execution, shard placement
consistency, and distributed index rebuilding without disruption.
This PR adds a regression test to verify the behavior of access methods
for partitioned and distributed tables, including:
- Creating partitioned tables with heap.
- Distributing tables using create_distributed_table.
- Switching access methods to columnar with ALTER TABLE.
- Validating access method inheritance for new partitions.
Relecant PG17 commit: https://github.com/postgres/postgres/commit/374c7a229
DESCRIPTION: Adds JSON_TABLE() support
PG17 has added basic `JSON_TABLE()` functionality
`JSON_TABLE()` allows `JSON` data to be converted into a relational view
and thus used, for example, in a `FROM` clause, like other tabular data.
We treat `JSON_TABLE` the same as correlated functions (e.g., recurring
tuples). In the end, for multi-shard `JSON_TABLE` commands, we apply the
same restrictions as reference tables (e.g., cannot perform a lateral
outer join when a distributed subquery references a (reference
table)/(json table) etc.)
Relevant PG17 commits:
[basic JSON
table](https://github.com/postgres/postgres/commit/de3600452), [nested
paths in json
table](https://github.com/postgres/postgres/commit/bb766cde6)
Onder had previously added json table support for PG15BETA1, but we
reverted that commit because json table was reverted in PG15.
ce7f1a530f
Previous relevant PG15Beta1 commit:
https://github.com/postgres/postgres/commit/4e34747c8
Therefore, I referred to Onder's commit for this commit as well, with a
few changes due to some differences between PG15/PG17:
1) In PG15Beta1, we had also `PLAN` clauses for `JSON_TABLE`
https://github.com/postgres/postgres/commit/fadb48b00, and Onder's
commit includes tests for those as well. However, `PLAN` nodes are _not_
added in PG17. Therefore, I didn't include the `json_table_select_only`
test, which had mostly queries involving `PLAN`. I only included the
last query from that test.
2) In PG15 timeline (Citus 11.1), we didn't support outer joins where
the outer rel is a recurring one and the inner one is a non-recurring
one. However, [Onur added support for that one in Citus
11.2](https://github.com/citusdata/citus/pull/6512), therefore I updated
the tests from Onder's commit accordingly.
3) PG17 json table has nested paths and columns, therefore I added a
test
with a distributed table, which is exactly the same as the one in
sqljson_jsontable in PG17.
https://github.com/postgres/postgres/commit/bb766cde6
This pull request also adds some basic tests on validation of SQL/JSON
constructor functions JSON(), JSON_SCALAR(), and JSON_SERIALIZE(),
and also SQL/JSON query functions JSON_EXISTS(), JSON_QUERY(), and
JSON_VALUE(). The relevant PG commits are the following:
[JSON(), JSON_SCALAR(),
JSON_SERIALIZE()](https://github.com/postgres/postgres/commit/03734a7fe)
[JSON_EXISTS(), JSON_VALUE(),
JSON_QUERY()](https://github.com/postgres/postgres/commit/6185c9737)
PG17 has added support for AT LOCAL operator
it converts the given time type to
time stamp with the session's TimeZone value as time zone. Here we add
tests that validate that we can use AT LOCAL at INSERT commands
Relevant PG commit:
https://github.com/postgres/postgres/commit/97957fdba
With the tests, we verify that we evaluate AT LOCAL at the coordinator
and then perform the insert remotely.
PG17 added support for
ALTER TABLE ... ALTER COLUMN ... SET EXPRESSION.
Relevant PG commit: https://github.com/postgres/postgres/commit/5d06e99a3
We currently don't support propagating this command for Citus tables.
It is added to future work.
This PR disallows `ALTER TABLE ... ALTER COLUMN ... SET EXPRESSION` on
all Citus table types (local, distributed, and partitioned distributed)
by adding an error check in `ErrorIfUnsupportedAlterTableStmt`. A new
regression test verifies that each table type fails with a consistent
error message when attempting to set an expression.
PG17 introduced ALTER TABLE ... SET ACCESS METHOD DEFAULT
This PR introduces and enforces an error check preventing ALTER TABLE
... SET ACCESS METHOD DEFAULT on both Citus local tables (added via
citus_add_local_table_to_metadata) and distributed/partitioned
distributed tables. The regression tests now demonstrate that each table
type raises an error advising users to explicitly specify an access
method, rather than relying on DEFAULT. This ensures consistent behavior
across local and distributed environments in Citus.
The reason why we currently don't support this is that we can't simply
propagate the command as it is, because the default table access method
may be different across Citus cluster nodes.
Relevant PG commit:
https://github.com/postgres/postgres/commit/d61a6cad6
These options already existed in PG17, and we support them and have
tests for them in `multi_copy.sql`.
In PG17, their capability was extended to specify ALL columns at once
using *.
Citus performs the COPY correctly, as is validated by the added tests in
this PR.
Relevant PG commit:
https://github.com/postgres/postgres/commit/f6d4c9cf1
Copy-pasting from Postgres documentation what these options do, such
that the reviewer may better understand the tests added:
`FORCE_NOT_NULL`: Do not match the specified columns' values against the
null string. In the default case where the null string is empty, this
means that empty values will be read as zero-length strings rather than
nulls, even when they are not quoted. If * is specified, the option will
be applied to all columns. This option is allowed only in `COPY FROM`,
and only when using `CSV` format.
`FORCE_NULL`: Match the specified columns' values against the null
string, even if it has been quoted, and if a match is found set the
value to `NULL`. In the default case where the null string is empty,
this converts a quoted empty string into `NULL`. If * is specified, the
option will be applied to all columns. This option is allowed only in
`COPY FROM`, and only when using `CSV` format.
`FORCE_NULL` and `FORCE_NOT_NULL` can be used simultaneously on the same
column. This results in converting quoted null strings to null values
and unquoted null strings to empty strings.
Explain it to me like I'm a 5-year-old, for a text column:
`FORCE_NULL` looks for empty strings and registers them as `NULL`
`FORCE_NOT_NULL` looks for null values and registers them as empty
strings.
PG17 added the new ON_ERROR option for COPY FROM. When this option is
specified, COPY skips soft errors and
continues copying.
Relevant PG commits:
-- https://github.com/postgres/postgres/commit/9e2d87011
-- https://github.com/postgres/postgres/commit/b725b7eec
I tried it locally with Citus tables.
Without further implementation, it doesn't work correctly.
Therefore, we error out for now, and add it to future work.
PG17 also added log_verbosity option, which controls the
amount of messages emitted during processing. This is
currently used in COPY FROM when ON_ERROR option is set to
ignore. Therefore, we error out for this option as well.
Relevant PG17 commit:
https://github.com/postgres/postgres/commit/f5a227895
DESCRIPTION: Propagates ALTER INDEX ALTER COLUMN SET STATISTICS DEFAULT
We automatically support this. Adding tests only.
We currently don't support ALTER TABLE ALTER COLUMN SET STATISTICS
Relevant PG commit:
https://github.com/postgres/postgres/commit/4f622503d
We are using `release-13.0` branch for both development and release, to
deliver PG17 support in Citus.
Afterwards, we will (probably) merge this branch into main.
Some potential changes for main branch, after we are done working on
release-13.0:
- Merge changes from `release-13.0` to `main`
- Figure out what changes were there on 12.2, move them to 13.1 version.
In a nutshell: rename `12.1--12.2` to `13.0--13.1` and fix issues.
- Set version to 13.1devel
In earlier versions of PostgreSQL, exclusion constraints were not
allowed on partitioned tables. This is why the error in your regression
test (ERROR: exclusion constraints are not supported on partitioned
tables) was raised in PostgreSQL 16. In PostgreSQL 17, exclusion
constraints are now allowed on partitioned tables, which is why the
error no longer appears when you attempt to add an exclusion constraint.
The constraint exclusion mechanism, described in the documentation,
relies on CHECK constraints to decide which partitions or child tables
need to be queried.
[CHECK
constraints](https://www.postgresql.org/docs/current/ddl-partitioning.html#DDL-PARTITIONING-CONSTRAINT-EXCLUSION)
```diff
-- Check "ADD EXCLUDE" errors out for partitioned table since the postgres does not allow it
ALTER TABLE AT_AddConstNoName.citus_local_partitioned_table ADD EXCLUDE(partition_col WITH =);
-ERROR: exclusion constraints are not supported on partitioned tables
-- Check "ADD CHECK"
SET client_min_messages TO DEBUG1;
ALTER TABLE AT_AddConstNoName.citus_local_partitioned_table ADD CHECK (dist_col > 0);
DEBUG: the constraint name on the shards of the partition is too long, switching to sequential and local execution mode to prevent self deadlocks: longlonglonglonglonglonglonglonglonglonglonglo_537570f5_5_check
DEBUG: verifying table "longlonglonglonglonglonglonglonglonglonglonglonglonglonglongabc"
DEBUG: verifying table "p1"
RESET client_min_messages;
SELECT con.conname
FROM pg_catalog.pg_constraint con
INNER JOIN pg_catalog.pg_class rel ON rel.oid = con.conrelid
INNER JOIN pg_catalog.pg_namespace nsp ON nsp.oid = connamespace
WHERE rel.relname = 'citus_local_partitioned_table';
conname
--------------------------------------------------
+ citus_local_partitioned_table_partition_col_excl
citus_local_partitioned_table_check
-(1 row)
+(2 rows)
```
This PR enhances `isolation_multiuser_locking.spec` test compatibility
across multiple PostgreSQL versions by handling differences in error
messages and behavior. Key updates include:
- **Error Message Handling:** Adjustments to manage version-specific
error messages, ensuring consistent test results.
- Modified to address variations in locking behavior across PostgreSQL
versions, ensuring test stability in multiuser scenarios.
- **REINDEX Behavior Adjustment**: This PR accounts for a behavioral
change introduced in PostgreSQL by commit ecb0fd337, which alters how
REINDEX interacts with system catalogs.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ecb0fd337
---------
Co-authored-by: Mehmet YILMAZ <mehmet.yilmaz@microsoft.com>
There is a crash when running vanilla tests because of the
`citus.hide_citus_dependent_objects` GUC. We turn on this GUC only for
the pg vanilla tests. This GUC runs the following function
`HideCitusDependentObjectsOnQueriesOfPgMetaTables`. This function
doesn't take into account the new `mergeJoinCondition`. I rewrote the
function such that it checks for merge join conditions as well.
Relevant PG commit:
https://github.com/postgres/postgres/commit/0294df2f1
The crash could be reproduced locally like the following:
```SQL
SET citus.hide_citus_dependent_objects TO on;
CREATE OR REPLACE FUNCTION
pg_catalog.is_citus_depended_object(oid,oid)
RETURNS bool
LANGUAGE C
AS 'citus', $$is_citus_depended_object$$;
-- try a system catalog
MERGE INTO pg_class c
USING (SELECT 'pg_depend'::regclass AS oid) AS j
ON j.oid = c.oid
WHEN MATCHED THEN
UPDATE SET reltuples = reltuples + 1
RETURNING j.oid;
CREATE VIEW classv AS SELECT * FROM pg_class;
MERGE INTO classv c
USING pg_namespace n
ON n.oid = c.relnamespace
WHEN MATCHED AND c.oid = 'pg_depend'::regclass THEN
UPDATE SET reltuples = reltuples - 1
RETURNING c.oid;
-- crash happens here
```
PostgreSQL 17 seems to have introduced improvements in how correlated
subqueries are handled during plan generation. Instead of generating a
trivial subplan with WHERE true, it now applies more specific filtering
(WHERE (key = 5)), which makes the execution plan more efficient.
https://github.com/postgres/postgres/commit/b262ad44
```
diff -dU10 -w /__w/citus/citus/src/test/regress/expected/local_table_join.out /__w/citus/citus/src/test/regress/results/local_table_join.out
--- /__w/citus/citus/src/test/regress/expected/local_table_join.out.modified 2024-11-05 09:53:50.423970699 +0000
+++ /__w/citus/citus/src/test/regress/results/local_table_join.out.modified 2024-11-05 09:53:50.463971296 +0000
@@ -1420,32 +1420,32 @@
) as subq_1
) as subq_2;
DEBUG: Wrapping relation "custom_pg_type" to a subquery
DEBUG: generating subplan 204_1 for subquery SELECT typdefault FROM local_table_join.custom_pg_type WHERE true
ERROR: direct joins between distributed and local tables are not supported
HINT: Use CTE's or subqueries to select from local tables and use them in joins
-- correlated sublinks are not yet supported because of #4470, unless we convert not-correlated table
SELECT COUNT(*) FROM distributed_table d1 JOIN postgres_table using(key)
WHERE d1.key IN (SELECT key FROM distributed_table WHERE d1.key = key and key = 5);
DEBUG: Wrapping relation "postgres_table" to a subquery
-DEBUG: generating subplan XXX_1 for subquery SELECT key FROM local_table_join.postgres_table WHERE true
+DEBUG: generating subplan 206_1 for subquery SELECT key FROM local_table_join.postgres_table WHERE (key OPERATOR(pg_catalog.=) 5)
```
Co-authored-by: Naisila Puka <37271756+naisila@users.noreply.github.com>
PostgreSQL 16 adds an extra condition (id IS NOT NULL) to the subquery.
This condition is likely used to ensure that no null values are
processed in the subquery. Instead of using the condition id IS NOT
NULL, PostgreSQL 17 generates the subplan with a trivial condition
(WHERE true), indicating that it does not need to explicitly check for
non-null values.
PostgreSQL 17 likely includes optimizations to handle null checks more
efficiently. The WHERE (id IS NOT NULL) condition that was present in
PostgreSQL 16 may now be considered redundant by the planner, as it is
implicitly handled by the query execution engine.
https://github.com/postgres/postgres/commit/b262ad44
```diff
SELECT
foo1.id
FROM
(SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo9,
(SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo8,
(SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo7,
(SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo6,
(SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo5,
(SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo4,
(SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo3,
(SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo2,
(SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo10,
(SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo1
WHERE
foo1.id = foo9.id AND
foo1.id = foo8.id AND
foo1.id = foo7.id AND
foo1.id = foo6.id AND
foo1.id = foo5.id AND
foo1.id = foo4.id AND
foo1.id = foo3.id AND
foo1.id = foo2.id AND
foo1.id = foo10.id AND
foo1.id = foo1.id
ORDER BY 1;
...
-DEBUG: generating subplan XXX_10 for subquery SELECT id FROM local_dist_join_mixed.local WHERE (id IS NOT NULL)
+DEBUG: generating subplan XXX_10 for subquery SELECT id FROM local_dist_join_mixed.local WHERE true
...
```
in regress test isolation_progress_monitoring, with an ORDER BY. The
implementation of get_progress() uses a tuplestore to hold the step and
progress values, and tuplestore does not provide any guarantee on the
ordering of the tuples so ORDER BY ensures stable test output. Also make
the output more user friendly by including the column names. Fixing
occasional failures seen in isolation_progress_monitoring.
- Adapted `pgmerge.sql` tests from PostgreSQL community's `merge.sql` to
Citus by converting tables into Citus local tables.
- Identified two new PostgreSQL 17 MERGE features (`RETURNING` support
and MERGE on updatable views) not yet supported by Citus.
- Implemented changes to detect unsupported features and raise clean
exceptions, ensuring pgmerge tests pass without diffs.
- Addressed breaking changes caused by `MERGE ... WHEN NOT MATCHED BY
SOURCE` restructuring, reducing diffs in pgmerge tests.
- Segregated unsupported test cases into `merge_unsupported.sql` to
maintain clarity and avoid large diffs in test files.
- Prepared the Citus MERGE planner to handle new PostgreSQL changes,
reducing remaining test discrepancies.
All merge tests now pass cleanly, with unsupported cases clearly
isolated.
Relevant PG commits:
c649fa24a
https://github.com/postgres/postgres/commit/c649fa24a
0294df2f1
https://github.com/postgres/postgres/commit/0294df2f1
---------
Co-authored-by: naisila <nicypp@gmail.com>
PG17 added support for identity columns in partitioned tables:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=699586315
A consequence is that a table with an identity column cannot be attached
as a partition. But Citus on Postgres 17 will generate identity column
for the partitions if the parent table has one (or more) identity
columns when propagating distributed table DDL to worker nodes, as
happens in the `generated_identity` regress test in #7768:
```
CREATE TABLE partitioned_table (
a bigint CONSTRAINT myconname GENERATED BY DEFAULT AS IDENTITY (START WITH 10 INCREMENT BY 10),
b bigint GENERATED ALWAYS AS IDENTITY (START WITH 10 INCREMENT BY 10),
c int
)
PARTITION BY RANGE (c);
CREATE TABLE partitioned_table_1_50 PARTITION OF partitioned_table FOR VALUES FROM (1) TO (50);
CREATE TABLE partitioned_table_50_500 PARTITION OF partitioned_table FOR VALUES FROM (50) TO (1000);
SELECT create_distributed_table('partitioned_table', 'a');
- create_distributed_table
----------------------------------------------------------------------
-
-(1 row)
-
+ERROR: table "partitioned_table_1_50" being attached contains an identity column "a"
+DETAIL: The new partition may not contain an identity column.
```
It is the Citus-generated ATTACH PARTITION statement that errors out,
because the Citus-generated CREATE TABLE for the partitions included
identity column definitions. The fix is straightforward - when
propagating the CREATE TABLE ddl for a partition of a table with an
identity column, don't include the identity column(s), they will be
inherited on attaching the partition. In Citus on Postgres 16 (or less)
partitions do not inherit identity; the partitions in the example would
not have any identity columns so it was not an issue previously.
Regress test `multi_explain` has two queries that have a different query
plan with PG17. Here is part of the plan diff for the query labelled
_Union and left join subquery pushdown_ in `multi_explain.sql` (for the
complete diff, search for `multi_explain`
[here](https://github.com/citusdata/citus/actions/runs/12158205599/attempts/1)):
```
-> Sort
Sort Key: ((users.composite_id).tenant_id), ((users.composite_id).user_id), subquery_2.hasdone, events.event_time
- -> Hash Left Join
- Hash Cond: (users.composite_id = subquery_2.composite_id)
- -> HashAggregate
- Group Key: ((users.composite_id).tenant_id), ((users.composite_id).user_id), users.composite_id, ('action=>1'::text), events.event_time
+ -> Nested Loop Left Join
+ Join Filter: (users.composite_id = subquery_2.composite_id)
+ -> Unique
+ -> Sort
+ Sort Key: ((users.composite_id).tenant_id), ((users.composite_id).user_id), users.composite_id, ('action=>1'::text), events.event_time
-> Append
```
The change is the same in both queries; a hash left join with subquery_1
on the outer and subquery_2 on the inner side of the join is now a
nested loop left join with subquery_1 on the outer and subquery_2 on the
inner; additionally, the chosen method of uniquifying the UNION in
subquery_1 has changed from hashed grouping to sort followed by unique,
as shown in the diff above.
The PG17 commit that caused this plan change is likely _[Fix MergeAppend
to more accurately compute the number of rows that need to be
sorted](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=9d1a5354f)_
because it impacts the estimated rows counts of UNION paths. Comparing a
costed plan of the query between PG16 and PG17 I noticed that with PG16
the rows estimate for the UNION in subquery_1 is 4, whereas with PG17
the rows estimate is 2. A lower rows estimate in the outer side of the
join may result in nested loop looking cheaper than hash join for the
left outer join, hence the plan change in the two queries where there is
a UNION on the outer side of a left outer join.
The proposed fix achieves a consistent plan across all supported
postgres versions by temporarily disabling nested loop join and sort for
the two impacted queries; the postgres optimizer selects hash join for
the outer left join and hashed aggregation for the UNION operation. I
investigated tweaking the queries, but was not able to arrive at a
consistent plan, and I believe the SQL operator (e.g. join, group by,
union) implementations are orthogonal to the intent of the test, so this
should be a satisfactory solution, particularly as it avoids introducing
a second alternative output file for `multi_explain`.
This PR addresses regress tests impacted by the introduction of [the
MAINTAIN privilege in
PG17](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ecb0fd337).
The impacted tests include `generated_identity`,
`create_single_shard_table`, `grant_on_sequence_propagation`,
`grant_on_foreign_server_propagation`, `single_node_enterprise`,
`multi_multiuser_master_protocol`,
`multi_alter_table_row_level_security`, `shard_move_constraints` which
show the following error:
```
SELECT start_metadata_sync_to_node('localhost', :worker_2_port);
- start_metadata_sync_to_node
----------------------------------------------------------------------
-
-(1 row)
-
+ERROR: unrecognized aclright: 16384
```
and `multi_multiuser_master_protocol`, where the `pg_class.relacl`
column has 'm' for MAINTAIN if applicable:
```
relname | rolname | relacl
---------------------+-------------+------------------------------------------------------------
trivial_full_access | full_access |
- trivial_postgres | postgres | {postgres=arwdDxt/postgres,full_access=arwdDxt/postgres}
+ trivial_postgres | postgres | {postgres=arwdDxtm/postgres,full_access=arwdDxtm/postgres}
```
The PR updates function `convert_aclright_to_string()` in
citus_ruleutils.c to include a case for `ACL_MAINTAIN`. Per the comment
on `convert_aclright_to_string()` in citus_ruleutils.c, it is a copy of
`convert_aclright_to_string()` in Postgres (where it is in
`src/backend/utils/adt/acl.c`), so requires updating to be consistent
with Postgres. With this change Citus can recognize the MAINTAIN
privilege, and will not emit the `unrecognized aclright` error. The PR
also adds an alternative goldfile for `multi_multiuser_master_protocol`.
Note that `convert_aclright_to_string()` in Postgres includes access
types SET and ALTER SYSTEM on system parameters (aka GUCs), added by
[this PG16
commit](https://github.com/postgres/postgres/commit/a0ffa885e). If Citus
were to have a requirement to support granting SET and ALTER SYSTEM we
would need to update `convert_aclright_to_string()` in citus_ruleutils.c
with SET and ALTER SYSTEM.
This fix ensures that the expected DEBUG error messages from the router
planner in `multi_router_planner`, `multi_router_planner_fast_path` and
`query_single_shard_table` are present with PG17.
In `query_single_shard_table` the diff:
```
SELECT COUNT(*) FROM citus_local_table t1
WHERE t1.b IN (
SELECT b+1 FROM nullkey_c1_t1 t2 WHERE t2.b = t1.a
);
-DEBUG: router planner does not support queries that reference non-colocated distributed tables
+DEBUG: Local tables cannot be used in distributed queries.
```
occurred because of[ this PG17
commit](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=9f1337639)
which enables the optimizer to pull up a correlated ANY subquery to a
join. The fix inhibits subquery pull up by including a volatile function
in the predicate involving the ANY subquery, preserving the pre-PG17
optimizer treatment of the query.
In the case of `multi_router_planner` and
`multi_router_planner_fast_path` the diffs:
```
-- partition_column is null clause does not prune out any shards,
-- all shards remain after shard pruning, not router plannable
SELECT *
FROM articles_hash a
WHERE a.author_id is null;
-DEBUG: Router planner cannot handle multi-shard select queries
+DEBUG: Creating router plan
```
are because of [this PG17
commit](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b262ad440),
which enables the optimizer to detect and remove redundant IS (NOT) NULL
expressions. The fix is to adjust the table definition so the column
used for distribution is not marked NOT NULL, thus preserving the
pre-PG17 query planning behavior.
Finallly, a rule is added to `normalize.sed` to ignore DEBUG logging in CREATE MATERIALIZED
VIEW AS statements introduced by [this PG17
commit](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b4da732fd64);
_when creating materialized views, use REFRESH logic to load data_, a
consequence of which is that with `client_min_messages` at `DEBUG2`
Postgres emits extra detail for CREATE MATERIALIZED VIEW AS statements.
```
CREATE MATERIALIZED VIEW mv_articles_hash_empty AS
SELECT * FROM articles_hash WHERE author_id = 1;
DEBUG: Creating router plan
DEBUG: query has a single distribution column value: 1
+DEBUG: drop auto-cascades to type multi_router_planner.pg_temp_61391
+DEBUG: drop auto-cascades to type multi_router_planner.pg_temp_61391[]
```
The rule can be changed to a normalization, or possibly dropped, when 17 becomes the minimum supported version.
PG17 regress sanity (#7653) fix; address diffs in vanilla tests
`create_index` and `privileges`. There is a change from `permission
denied` to `must be owner of`, seen in create_index:
```
@@ -2970,21 +2970,21 @@
REINDEX TABLE pg_toast.pg_toast_1260;
ERROR: permission denied for table pg_toast_1260
REINDEX INDEX pg_toast.pg_toast_1260_index;
-ERROR: permission denied for index pg_toast_1260_index
+ERROR: must be owner of index pg_toast_1260_index
```
and privileges:
```
@@ -2945,41 +2945,43 @@
ERROR: permission denied for table maintain_test
REINDEX INDEX maintain_test_a_idx;
-ERROR: permission denied for index maintain_test_a_idx
+ERROR: must be owner of index maintain_test_a_idx
REINDEX SCHEMA reindex_test;
REINDEX INDEX maintain_test_a_idx;
+ERROR: must be owner of index maintain_test_a_idx
REINDEX SCHEMA reindex_test;
```
The fix updates function `RangeVarCallbackForReindexIndex()` in
`index.c` with changes made by the introduction of the [MAINTAIN
privilege in
PG17](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ecb0fd337)
to the function `RangeVarCallbackForReindexIndex()` in `indexcmds.c`.
The code is under a Postgres 17 version directive, which can be removed
when 17 becomes the oldest supported Postgres version.
This PR fixes diffs in `columnnar_chunk_filtering` and `columnar_paths`
tests.
In `columnnar_chunk_filtering` an expression `(NOT (SubPlan 1))` changed
to `(NOT (ANY (a = (SubPlan 1).col1)))`. This is due to [aPG17
commit](https://github.com/postgres/postgres/commit/fd0398fc) that
improved how scalar subqueries (InitPlans) and ANY subqueries (SubPlans)
are EXPLAINed in expressions. The fix uses a helper function which
converts the PG17 format to the pre-PG17 format. It is done this way
because pre-PG17 EXPLAIN does not provide enough context to convert to
the PG17 format. The helper function can (and should) be retired when 17
becomes the minimum supported PG.
In `columnar_paths`, a merge join changed to a hash join. This is due to
[this PG17
commit](f7816aec23),
which improved the PG optimizer's ability to estimate the size of a CTE
scan. The impacted query involves a CTE scan with a point predicate
`(a=123)` and before the change the CTE size was estimated to be 5000,
but with the change it is correctly (given the data in the table)
estimated to be 1, making hash join a more attractive join method. The
fix is to have an alternative goldfile for pre-PG17. I tried, but was
unable, to force a specific kind of join method using the GUCs
(`enable_nestloop`, `enable_hashjoin`, `enable_mergejoin`), but it was
not possible to obtain a consistent plan across all supported PG
versions (in some cases the join inputs switched sides).
There are two commits in this PR:
1) Remove domain_default column since it has been removed from PG17
Relevant PG commit:
78806a9509
78806a95095c4fb9230a441925244690d9c07d23
2) pg_stat_statements reset output diff fix
pg_stat_statements reset output changed in PG17, fix idea from
Relevant PG commits:
6ab1dbd26b
6ab1dbd26bbf307055d805feaaca16dc3e750d36
Test `tableam` expects that this CREATE TABLE statement: `CREATE TABLE
test_partitioned(id int, p int, val int) PARTITION BY RANGE (p) USING
fake_am;`
will produce this error:
`specifying a table access method is not supported on a partitioned
table`
but as of [this PG
commit](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=374c7a229)
it is possible to specify an access method on a partitioned table. This
fix moves the CREATE TABLE statement to pg17, and adds an additional
test to show parent access method is inherited.
Disable DDL propagation for the vanilla test suite. This enables the
vanilla `database ` test to pass, where previously it was correctly
returning `ERROR: unrecognized ALTER DATABASE option: tablespace`
because release-13.0 does not propagate this ALTER DATABASE variant.
We (Citus team) discussed cherry picking
[#7253](https://github.com/citusdata/citus/pull/7253) from main to
release-13.0 because it does propagate ALTER DATABASE tablespace option
(as well as a couple of others) but decided fixing the regress test was
not the proper context for that. The fix disables
`citus.enable_metadata_sync` when running vanilla, we discussed
disabling `citus.enable_create_database_propagation` but this is not in
release-13.0.
Preserve the test error message by adjusting the query so that PG17
cannot pull it up to a join. Another instance of a subquery that can be
pulled up to a join with PG17 (#7745)
This should have been fixed in, but slipped by, #7745
In PG17, Auto-generated array types, multirange types, and relation
rowtypes
are treated as dependent objects, hence changing the output of the
print_extension_changes function.
Relevant PG commit:
e5bc9454e527b1cba97553531d8d4992892fdeef
e5bc9454e5
Here we create a table with only the basic extension types
in order to avoid printing extra ones for now.
This can be removed when we drop PG16 support.
https://github.com/citusdata/citus/actions/runs/11960253650/attempts/1#summary-33343972656
```diff
| table pg_dist_rebalance_strategy
+ | type citus.distribution_type[]
+ | type citus.pg_dist_object
+ | type pg_dist_shard
+ | type pg_dist_shard[]
+ | type pg_dist_shard_placement
+ | type pg_dist_shard_placement[]
+ | type pg_dist_transaction
+ | type pg_dist_transaction[]
| view citus_dist_stat_activity
| view pg_dist_shard_placement
```
This work was already done by @m3hm3t and approved as part of
https://github.com/citusdata/citus/pull/7722
I separated it in this PR since the previous one contained other changes
which we don't currently want to merge.
Relevant PG commit:
---------
Co-authored-by: Mehmet YILMAZ <mehmety87@gmail.com>
A recent Postgres commit (*) that refactored error messages is the cause
of the diffs in pg16 regress test when running Citus on Postgres 17. The
fix changes the pg16 goldfile and includes a normalization rule for the
error messages so pg16 will pass when running with version 16 of
Postgres.
(*)
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=498ee9ee2f
PG17 changed how scalar subquery outputs appear in EXPLAIN output (*).
This commit changes impacted regress goldfiles to the PG17 format, and
adds a helper function to covert pre-PG17 plans to the PG17 format. The
conversion is required when testing Citus on pgversions prior to 17. The
helper function can and should be removed when 17 becomes the minimum
supported version.
(*)
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=fd0398fcb
Fix Test Failure in subquery_in_where, set_operations, dml_recursive in
PG17 #7741
The test failures are caused by[ this commit in
PG17](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=9f1337639),
which enables correlated subqueries to be pulled up to a join. Prior to
this, the correlated subquery was implemented as a subplan. In citus, it
is not possible to pushdown a correlated subplan, but with a different
plan in PG17 the query can be executed, per the test diff from
`subquery_in_where`:
```
37,39c37,41
< DEBUG: generating subplan XXX_1 for CTE event_id: SELECT user_id AS events_user_id, "time" AS events_time, event_type FROM public.events_table
< DEBUG: Plan XXX query after replacing subqueries and CTEs: SELECT count(*) AS count FROM ...
< ERROR: correlated subqueries are not supported when the FROM clause contains a CTE or subquery
---
> count
> ---------------------------------------------------------------------
> 0
> (1 row)
>
```
This is because with pg17 `= ANY subquery` in the queries can be
implemented as a join, instead of as a subplan filter on a table scan.
For example, `SELECT * FROM test a WHERE x IN (SELECT x FROM test b
UNION SELECT y FROM test c WHERE a.x = c.x) ORDER BY 1,2` (from
set_operations) has this plan in pg17; note that the subquery is the
inner side of a nested loop join:
```
┌───────────────────────────────────────────────────┐
│ QUERY PLAN │
├───────────────────────────────────────────────────┤
│ Sort │
│ Sort Key: a.x, a.y │
│ -> Nested Loop │
│ -> Seq Scan on test a │
│ -> Subquery Scan on "ANY_subquery" │
│ Filter: (a.x = "ANY_subquery".x) │
│ -> HashAggregate │
│ Group Key: b.x │
│ -> Append │
│ -> Seq Scan on test b │
│ -> Seq Scan on test c │
│ Filter: (a.x = x) │
└───────────────────────────────────────────────────┘
```
and this plan in pg16 (and previous pg versions); the subquery is a
correlated subplan filter on a table scan:
```
┌───────────────────────────────────────────────┐
│ QUERY PLAN │
├───────────────────────────────────────────────┤
│ Sort │
│ Sort Key: a.x, a.y │
│ -> Seq Scan on test a │
│ Filter: (SubPlan 1) │
│ SubPlan 1 │
│ -> HashAggregate │
│ Group Key: b.x │
│ -> Append │
│ -> Seq Scan on test b │
│ -> Seq Scan on test c │
│ Filter: (a.x = x) │
└───────────────────────────────────────────────┘
```
The fix Modifies the queries causing the test failures so that an ANY
subquery is not folded to a join, preserving the expected output of the
tests. A similar approach was taken for existing regress tests in the[
postgres
commit](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=9f1337639).
See the `join `regress test, for example.
We also add pg17 specific tests that leverage this improvement in Postgres
with Citus distributed planning as well.
Onur Tirtir
Naisila Puka
Mehmet YILMAZ
Colm
Teja Mupparti
Colm