Re: SQL:2011 application time - Mailing list pgsql-hackers

On Wed, 4 Dec 2024 at 16:45, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 26.11.24 13:18, Peter Eisentraut wrote:
> > I think this is the right idea, but after digging around a bit more, I
> > think more could/should be done.
> >
> > After these changes, the difference between
> > get_equal_strategy_number_for_am() and get_equal_strategy_number() is
> > kind of pointless.  We should really just use
> > get_equal_strategy_number() for all purposes.
> >
> > But then you have the problem that IsIndexUsableForReplicaIdentityFull()
> > doesn't have the opclass IDs available in the IndexInfo structure.  You
> > appear to have worked around that by writing
> >
> > +   if (get_equal_strategy_number_for_am(indexInfo->ii_Am, InvalidOid)
> > == InvalidStrategy)
> >
> > which I suppose will have the same ultimate result as before that patch,
> > but it seems kind of incomplete.
> >
> > I figure this could all be simpler if
> > IsIndexUsableForReplicaIdentityFull() used the index relcache entry
> > directly instead of going the detour through IndexInfo.  Then we have
> > all the information available, and this should ultimately all work
> > properly for suitable GiST indexes as well.
> >
> > 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.

I started having look at the patch, here are some comments while doing
the initial review:
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');

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
+#

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;
+       }

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

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

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))"
+);
+

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");

Regards,
Vignesh