Re: table-write trigger can bypass ATPrepChangePersistence

On Fri, 18 Sept 2026 at 13:02, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Fri, Sep 18, 2026 at 4:43 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > Now for a possible fix. Your patch puts the recheck right after the
> > event trigger fires. That is inside the per-table loop in
> > ATRewriteTables(). The DEFAULT expression runs later in that same
> > loop, during the actual rewrite of that table in ATRewriteTable(). So
> > the recheck happens too early for this case.
> >
> > A better place is after the whole loop finishes. This means every
> > table has already been rewritten. All DEFAULT and CHECK functions have
> > already run. Only then do we check persistence again, for every table
> > in the list.
> >
>
> Here's a patch with this change incorporated.
>
Hi Kuroda-san, Ajin,

I reviewed the v2 patch. Here are some comments:
1. There is a trailing whitespace error:
Applying: Recheck table persistence after table_rewrite triggers
.git/rebase-apply/patch:28: trailing whitespace.
*/
warning: 1 line adds whitespace errors.

2. Do we need to acquire an AccessShareLock here?
+ATRewriteChangePersistence(AlteredTableInfo *tab)
+{
+ Relation rel;
+ bool toLogged;
+
+ rel = table_open(tab->relid, AccessShareLock);
+ toLogged = (tab->newrelpersistence == RELPERSISTENCE_PERMANENT);

ALTER TABLE ... SET LOGGED/UNLOGGED already acquires and retains an
AccessExclusiveLock on the target relation:
lockmode = AlterTableGetLockLevel(atstmt->cmds);
relid = AlterTableLookupRelation(atstmt, lockmode);
Here, lockmode is AccessExclusiveLock.

Should we use NoLock instead of AccessShareLock? This would also be
consistent with the other phase 3 relation opens in ATRewriteTables().

3. Should we also add tests for DDL executed by DEFAULT/CHECK expressions to
event_trigger.sql?

Thanks,
Shlok Kyal