Re: BUG #17789: process_pgfdw_appname() fails for autovacuum workers

At Tue, 14 Feb 2023 17:02:45 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in 
> On Tue, Feb 14, 2023 at 2:05 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > At Mon, 13 Feb 2023 05:00:01 +0000, PG Bug reporting form <noreply@postgresql.org> wrote in
> > > CREATE FOREIGN TABLE ft1 (b int) INHERITS (t1)
> > >         SERVER LOOPBACK OPTIONS (table_name 'anytable');
> >
> > Yeah, the autovacuum processes t1 and then its child foreign table
> > ft1. MyProcPort is NULL on autovacuum processes. This doesn't happen
> > for partitioned tables.
> >
> > > #5  0x0000564b2ecc55c8 in ExceptionalCondition
> > > (conditionName=conditionName@entry=0x7fb1feb3f81e "MyProcPort != NULL",
> > >     errorType=errorType@entry=0x7fb1feb3e700 "FailedAssertion",
> > > fileName=fileName@entry=0x7fb1feb3f75a "option.c",
> > >     lineNumber=lineNumber@entry=464) at assert.c:69
> > > #6  0x00007fb1feb31b30 in process_pgfdw_appname (appname=<optimized out>) at
> > > option.c:464
> >
> > Commit 6e0cb3dec1 overlooked the case of autovacuum analyzing foreign
> > tables as an inheritance child. We thought NULL MyProcPort was
> > impossible in the discussion for the patch. Using %d and %u in
> > postgres_fdw.application_name we hit the bug.
> >
> > The following change fixes the bug.
> 
> The change seems to work fine.

Thanks for veryfiying that!

> > - Is it okay to use '-' as %u (user name) and %d (databsae name) in
> >   the NULL MyProcPort case?
> 
> Another idea seems to not display anything in this case, which is
> consistent with what log_status_format() does.

As you said, when MyProcPort is NULL, the function won't insert
anything unless padding is specified. But in this case, padding isn't
even a thing. So it seems best to not insert anything.

By the way, when we designed that part, we first concluded that the
user name and database name cannot be NULL before we later concluded
that MyProcPort cannot be NULL. However, log_status_format() takes
into account the case where MyProcPort stores NULL as user and
database names.  Do you think we should still ignore the case of NULL
user name and database names, even though our assumption about
MyProcPort was found to be incorrect? In that case,
log_status_format() replaces, for example, %u with '[unknown]'.

> > - Do we need tests for the case? They need to wait for autovacuum to run.
> 
> Not sure it's worth having tests for that as the fix is obvious.

Thanks you for sharing your thoughts.  I'll go with that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center