On Mon, Sep 2, 2019 at 6:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > As far as 4) goes, I think the code in ExtractReplicaIdentity is pretty
> > duff anyway, because it doesn't bother to check for the defined failure
> > return for RelationIdGetRelation. But if we're concerned about the
> > cost of recalculating this stuff per-row, couldn't we cache it a little
> > better? It should be safe to assume the set of index columns isn't
> > changing intra-query.
> > ... in fact, isn't all the infrastructure for that present already?
> > Why is this code looking directly at the index at all, rather than
> > using the relcache's rd_idattr bitmap?
>
> Here's a proposed patch along those lines. It fixes Hadi's original
> crash case and passes check-world.
Agree that this patch would be a better solution for Hadi's report,
although I also agree that the situation with index locking for DELETE
isn't perfect.
> I'm a bit suspicious of the exclusion for idattrs being empty, but
> if I remove that, some of the contrib/test_decoding test results
> change. Anybody want to comment on that? If that's actually an
> expected situation, why is there an elog(DEBUG) in that path?
ISTM that the exclusion case may occur with the table's replica
identity being REPLICA_IDENTITY_DEFAULT and there being no primary
index defined, in which case nothing needs to get logged.
The elog(DEBUG) may just be a remnant from the days when this was
being developed. I couldn't find any notes on it though in the
archives [1] though.
Thanks,
Amit
[1] https://www.postgresql.org/message-id/flat/20131204155510.GO24801%40awork2.anarazel.de