Re: PATCH: Add REINDEX tag to event triggers

On Tue, Dec 05, 2023 at 12:49:12AM +0800, jian he wrote:
> On Mon, Dec 4, 2023 at 11:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:
>> Please look at the assertion failure triggered when REINDEX processed by an event trigger:
>> CREATE FUNCTION etf() RETURNS EVENT_TRIGGER AS $$ BEGIN PERFORM 1; END $$ LANGUAGE plpgsql;
>> CREATE EVENT TRIGGER et ON ddl_command_end EXECUTE FUNCTION etf();
>> CREATE TABLE t (i int);
>> REINDEX (CONCURRENTLY) TABLE t;
>
> In indexcmd.c, function, ReindexRelationConcurrently
> if (indexIds == NIL)
> {
> PopActiveSnapshot();
> return false;
> }
>
> So there is no snapshot left, then PERFORM 1; need a snapshot.

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