Re: Logical replication can lose an update after concurrent index invalidation

Zhijie, Amit, thanks for the reviews!

> we shall mention in the comments atop the old function that it should
> not be used in new code anymore

Done.

> The other two GetRelationIdentityOrPK callers in logicalrep also look
> worth adjusting on top of the current patch's new flag (maybe in
> followup patches)

check_relation_updatable() is in this patch - it is a one-line change
conflict.c is not so fixing it means much more work to do. It gets an
XXX comment for now.

> Can we consider passing the LogicalRepRelMapEntry * instead that
> collapses two parameters into one?

Done for FindReplTupleInLocalRel(), FindDeletedTupleInLocalRel() and
apply_handle_delete_internal().
apply_handle_update_internal() just loses the Oid instead: its only
caller is the non-partitioned path, where the entry is
edata->targetRel.

> How about naming struct member as idxisreplident

Done.

> [the Assert] verifies nothing when isidentity is true

I took your (b) as is.

> I think this deserves a update in .abi-compliance-history but [...] we
> can add it afterwards but certainly note down the need of same in the
> commit message

The commit message now says so.

> how can we ascertain that we need a new API for
> RelationFindReplTupleByIndex in back-branches?

spock does call it - [0].

I haven't checked back-branch commits (AI did them + ran the tests),
but here is one note from that process:

The back-branch patches are against the current tips. One porting note
for 16 and 17: their cross-partition UPDATE path has a second
apply_handle_delete_internal() call that master no longer has; it is
converted as well.

[0]: https://github.com/pgEdge/spock/blob/main/src/spock_apply_heap.c#L417

Regards,
Mikhail Nikalayeu