Re: REL_12_STABLE crashing with assertion failure in ExtractReplicaIdentity

Hi,

On 2019-09-01 16:50:00 -0400, Tom Lane 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.

I agree that it ought to be more efficent - but also about as equally
safe? I.e. if the previous code wasn't safe, the new code wouldn't be
safe either? As in, we're "just" avoiding the assert, but not increasing
safety?

> ... 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?

No idea, that's too long ago :(

> 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?

I think Amit's explanation here is probably accurate.

Greetings,

Andres Freund