Re: BUG #15114: logical decoding Segmentation fault

On 30/10/2018 07:28, Michael Paquier wrote:
> 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

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.

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

Well, considering we don't test for this misuse of IMMUTABLE anywhere
else either, meh, but okay.

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

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

I don't agree, about that, INDEX_ATTR_BITMAP_IDENTITY_KEY is perfectly
safe when used correctly. Not sure what you mean regarding unsafety of
rd_idattr but I don't see why it should not be safe.

> 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.
If we want to put additional Asserts it should be about having snapshot
not about output plugin IMHO.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services