Re: BUG #15114: logical decoding Segmentation fault

On Mon, Oct 29, 2018 at 04:03:31PM +0100, Petr Jelinek wrote:
> The reason why it breaks for OP is that his IMMUTABLE function is not
> actually IMMUTABLE, that's also why we don't see it reported much.

Thanks, Petr. I have missed that part. Now I can see the failure
easily. Please note that on the receiver side you have the same
problem, RelationGetIndexAttrBitmap() is still being used in
logicalrep_rel_open() so that crashes the receiver:
#18 0x0000563cc278baa4 in RelationGetIndexPredicate
(relation=0x7f9764cdc050) at relcache.c:4720
#19 0x0000563cc22ccdd6 in BuildIndexInfo (index=0x7f9764cdc050) at
index.c:1789
#20 0x0000563cc278be4f in RelationGetIndexAttrBitmap
(relation=0x7f9764de52e8, attrKind=INDEX_ATTR_BITMAP_IDENTITY_KEY) at
relcache.c:4921
#21 0x0000563cc25a6b0c in logicalrep_rel_open (remoteid=16388,
lockmode=3) at relation.c:313

> Given this I am not sure if we want to add test for this as it's
> something one should not do because it has unpredictable side effects,
> but if you think it will be useful anyway I can whip something out.

Hmm. People should not do that but they can do it, so this thread is
telling is to be careful. Adding new objects in the schema of the
publisher and the receiver would be also cheap. A test like that is not
something we would see but I would suggest to stress the new routine on
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? What is
proposed here needs more thoughts I think. Something that we could
consider on HEAD is to remove INDEX_ATTR_BITMAP_IDENTITY_KEY as it is
visibly unsafe. rd_idattr is also an unsafe concept.

Putting a safeguard in RelationGetIndexAttrBitmap() so as it generates
an ERROR if called in an output plugin, or just assert would be nice,
however such a check would need to use MyReplicationSlot (I don't recall
something else), and the limitations could just be documented as well.
--
Michael