From: | Peter Eisentraut <peter(at)eisentraut(dot)org> |
---|---|
To: | Paul Jungwirth <pj(at)illuminatedcomputing(dot)com> |
Cc: | jian he <jian(dot)universality(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: SQL:2011 application time |
Date: | 2024-12-04 11:15:40 |
Message-ID: | e8e51e74-10d7-410d-a1d1-8ec681d8436a@eisentraut.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
Attachment | Content-Type | Size |
---|---|---|
v45.2-0001-Improve-internal-logical-replication-error-for.patch | text/plain | 1.5 KB |
v45.2-0002-Replace-get_equal_strategy_number_for_am-by-ge.patch | text/plain | 5.2 KB |
v45.2-0003-Make-the-conditions-in-IsIndexUsableForReplica.patch | text/plain | 4.8 KB |
v45.2-0004-Support-for-GiST-in-get_equal_strategy_number.patch | text/plain | 2.1 KB |
v45.2-0005-Tests-for-logical-replication-with-temporal-ke.patch | text/plain | 26.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Yugo NAGATA | 2024-12-04 11:20:57 | Re: psql: Add leakproof field to \dAo+ meta-command results |
Previous Message | Kirill Reshke | 2024-12-04 11:12:07 | Re: Implement waiting for wal lsn replay: reloaded |