Re: SQL:2011 application time

On 12/4/24 03:15, Peter Eisentraut wrote:
>> I have attached three patches that show how that could be done. (This would work in conjunction
>> with your new tests. (Although now we could also test GiST with replica identity full?))
>>
>> The comment block for IsIndexUsableForReplicaIdentityFull() makes a bunch of claims that are not
>> all explicitly supported by the code. The code doesn't actually check the AM, this is all only
>> done indirectly via other checks. The second point (about tuples_equal()) appears to be slightly
>> wrong, because while you need an equals operator from the type cache, that shouldn't prevent you
>> from also using a different index AM than btree or hash for the replica identity index. And the
>> stuff about amgettuple, if that is important, why is it only checked for assert builds?
>
> I did some more work on this approach, with the attached patches resulting. This is essentially
> what I'm describing above, which in turn is a variation of your patch v45-0001-Fix-logical-
> replication-for-temporal-tables.patch, with your tests added at the end.
>
> I also did some more work on IsIndexUsableForReplicaIdentityFull() to make the various claims in the
> comments reflected by actual code. With all of this, it can now also use gist indexes on the
> subscriber side in cases of REPLICA IDENTITY FULL. This isn't immediately visible in the tests, but
> you can see that the tests are using it internally by adding debugging elogs or something like that.
>
> Altogether, I think this fixes the original problem of temporal keys not being handled properly in
> logical replication subscribers, and it makes things less hardcoded around btree and hash in general.
>
> Please review.

These five patches all look good to me.

Note that my tests already include a section for REPLICA IDENTITY FULL, which passed. But the
subscriber was using a SeqScan to look up tuples to update.

Here are the steps (mostly just because it was confusing for me at first): First in
FindUsableIndexForReplicaIdentityFull, we would call IsIndexUsableForReplicaIdentityFull, get back
false, and decide there was no index to use. Then in FindReplTupleInLocalRel, localidxoid was 0, so
we woudln't call IsIndexUsableForReplicaIdentityFull at all.

After applying the five patches, I can see that we choose the index and call
IsIndexUsableForReplicaIdentityFull from both sites. This should make applying changes a lot faster.

Here are those patches again, but incoporating Vignesh's feedback:

On 12/5/24 01:39, vignesh C wrote:
> 1) wait_for_catchup and data validation can be done after insertion
> itself, update and delete error validation can happen later:
> +($result, $stdout, $stderr) = $node_publisher->psql('postgres',
> + "DELETE FROM temporal_no_key WHERE id = '[3,4)'");
> +is( $stderr,
> + qq(psql:<stdin>:1: ERROR: cannot delete from table
> "temporal_no_key" because it does not have a replica identity and
> publishes deletes
> +HINT: To enable deleting from the table, set REPLICA IDENTITY using
> ALTER TABLE.),
> + "can't DELETE temporal_no_key DEFAULT");
> +
> +$node_publisher->wait_for_catchup('sub1');
> +
> +$result = $node_subscriber->safe_psql('postgres',
> + "SELECT * FROM temporal_no_key ORDER BY id, valid_at");
> +is( $result, qq{[1,2)|[2000-01-01,2010-01-01)|a
> +[2,3)|[2000-01-01,2010-01-01)|a
> +[3,4)|[2000-01-01,2010-01-01)|a
> +[4,5)|[2000-01-01,2010-01-01)|a}, 'replicated temporal_no_key DEFAULT');

I'd like to keep the order of tests the same for all scenarios, and sometimes update+delete succeed
and sometimes they fail. The data validation includes changes caused by the update+delete. Even when
they fail, putting the data validation at the end shows that they had no effect.

> 2) Copyright need not mention "2021-"
> diff --git a/src/test/subscription/t/034_temporal.pl
> b/src/test/subscription/t/034_temporal.pl
> new file mode 100644
> index 00000000000..0f501f1cee8
> --- /dev/null
> +++ b/src/test/subscription/t/034_temporal.pl
> @@ -0,0 +1,673 @@
> +
> +# Copyright (c) 2021-2024, PostgreSQL Global Development Group
> +
> +# Logical replication tests for temporal tables
> +#

Okay.

> 3) This statement seems very long in a single line, could we split it
> into multiple lines:
> @@ -844,6 +842,15 @@ IsIndexUsableForReplicaIdentityFull(Relation
> idxrel, AttrMap *attrmap)
>
> Assert(idxrel->rd_index->indnatts >= 1);
>
> + indclass = (oidvector *)
> DatumGetPointer(SysCacheGetAttrNotNull(INDEXRELID,
> idxrel->rd_indextuple, Anum_pg_index_indclass));
> +
> + /* Ensure that the index has a valid equal strategy for each
> key column */
> + for (int i = 0; i < idxrel->rd_index->indnkeyatts; i++)
> + {
> + if (get_equal_strategy_number(indclass->values[i]) ==
> InvalidStrategy)
> + return false;
> + }

Reformatted.

> 4) The commit message had a small typo, should "fulfull" be "fulfill":
> IsIndexUsableForReplicaIdentityFull() described a number of conditions
> that a suitable index has to fulfull. But not all of these were

Fixed.

> 5) temporal_no_key table is not dropped:
> +$result = $node_subscriber->safe_psql('postgres',
> + "SELECT * FROM temporal_unique ORDER BY id, valid_at");
> +is( $result, qq{[1,2)|[2000-01-01,2010-01-01)|a
> +[2,3)|[2000-01-01,2010-01-01)|a
> +[3,4)|[2000-01-01,2010-01-01)|a
> +[4,5)|[2000-01-01,2010-01-01)|a}, 'replicated temporal_unique NOTHING');
> +
> +# cleanup
> +
> +$node_publisher->safe_psql('postgres', "DROP TABLE temporal_pk");
> +$node_publisher->safe_psql('postgres', "DROP TABLE temporal_unique");
> +$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
> +$node_subscriber->safe_psql('postgres', "DROP TABLE temporal_pk");
> +$node_subscriber->safe_psql('postgres', "DROP TABLE temporal_unique");
> +$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");
> +
> +
> +done_testing();
> --
> 2.47.1

Fixed as part of #7 below.

> 6) Since this is common to first and last test we can have it in a
> subroutine and use it for both:
> +# create tables on publisher
> +
> +$node_publisher->safe_psql('postgres',
> + "CREATE TABLE temporal_no_key (id int4range, valid_at
> daterange, a text)"
> +);
> +
> +$node_publisher->safe_psql('postgres',
> + "CREATE TABLE temporal_pk (id int4range, valid_at daterange, a
> text, PRIMARY KEY (id, valid_at WITHOUT OVERLAPS))"
> +);
> +
> +$node_publisher->safe_psql('postgres',
> + "CREATE TABLE temporal_unique (id int4range NOT NULL, valid_at
> daterange NOT NULL, a text, UNIQUE (id, valid_at WITHOUT OVERLAPS))"
> +);
> +
> +# create tables on subscriber
> +
> +$node_subscriber->safe_psql('postgres',
> + "CREATE TABLE temporal_no_key (id int4range, valid_at
> daterange, a text)"
> +);
> +
> +$node_subscriber->safe_psql('postgres',
> + "CREATE TABLE temporal_pk (id int4range, valid_at daterange, a
> text, PRIMARY KEY (id, valid_at WITHOUT OVERLAPS))"
> +);
> +
> +$node_subscriber->safe_psql('postgres',
> + "CREATE TABLE temporal_unique (id int4range NOT NULL, valid_at
> daterange NOT NULL, a text, UNIQUE (id, valid_at WITHOUT OVERLAPS))"
> +);
> +

Done.

> 7) Similarly the drop tables can be moved to a subroutine and used as
> it is being used in multiple tests:
> +# cleanup
> +
> +$node_publisher->safe_psql('postgres', "DROP TABLE temporal_no_key");
> +$node_publisher->safe_psql('postgres', "DROP TABLE temporal_pk");
> +$node_publisher->safe_psql('postgres', "DROP TABLE temporal_unique");
> +$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
> +$node_subscriber->safe_psql('postgres', "DROP TABLE temporal_no_key");
> +$node_subscriber->safe_psql('postgres', "DROP TABLE temporal_pk");
> +$node_subscriber->safe_psql('postgres', "DROP TABLE temporal_unique");
> +$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");

Done.

Thanks!

Rebased to 3220ceaf77.

Yours,

--
Paul ~{:-)
pj(at)illuminatedcomputing(dot)com