Re: PATCH: Add REINDEX tag to event triggers

Hi Michael,

05.12.2023 02:45, Michael Paquier wrote:
> Popping a snapshot at this stage when there are no indexes has been a
> decision taken by the original commit in 5dc92b844e68 because we had
> no need for it yet, but we may do now depending on the function
> triggered.  I have been looking at the whole stack and something like
> the attached to make a pop conditional seems to be sufficient to
> satisfy all the cases I've been able to come up with, including the
> one reported here.
>
> Alexander, do you see any hole in that?  Perhaps the snapshot push
> should be more aggressive at the end of ReindexRelationConcurrently()
> as well (in the last transaction when rebuilds happen)?

Thank you for the fix proposed!

I agree with it. I had worried a bit about ReindexRelationConcurrently()
becoming twofold for callers (it can leave the snapshot or pop it), but I
couldn't find a way to hide this twofoldness inside without adding more
complexity. On the other hand, ReindexRelationConcurrently() now satisfies
EnsurePortalSnapshotExists() in all cases.

Best regards,
Alexander