Re: BUG #15114: logical decoding Segmentation fault

On Tue, Oct 30, 2018 at 08:45:57AM +0100, Petr Jelinek wrote:
> The receiver side *should* actually use RelationGetIndexAttrBitmap, the
> problem there seems to be rather than we setup snapshot few lines of
> code too late in worker.c.

Does the worker need this much information though?  It just needs to
know about the replication index itself and its columns, not about all
the other indexes.  It may make sense to unify both code paths to behave
similarly.

>> both sides.  In the patch, something which makes me uncomfortable is
>> that get_rel_idattr() is a duplicate of the code paths taken by
>> RelationGetIndexAttrBitmap() when INDEX_ATTR_BITMAP_IDENTITY_KEY gets
>> used to fill in the relation cache.  In this case, wouldn't we want to
>> just fetch the replica index instead of the full index list?
>
> I don't follow, we *do* only fetch replica index in the new function.

I am wondering if both things should be unified, or simply if we should
just remove completely the dependency on the cached Relation in
get_rel_idattr().  One function able to overwrite what the other does is
weird.

> If we want to put additional Asserts it should be about having snapshot
> not about output plugin IMHO.

That looks like a good idea to me in any case, and that's a good thing
for HEAD.  Something like Assert(ActiveSnapshotSet()) perhaps?  I have
not thought much about it to be honest.  Let's tackle that with a
HEAD-only, separate patch.
--
Michael