MIME-Version: 1.0
References: <CAExHW5vvhhtdv-LctC5Wz-yuyh1me3KtvyuOqX48BRQicDGTJg@mail.gmail.com>
 <20230717142452.74yml7mfqkypfixp@alvherre.pgsql> <CAA4eK1+A3h9rmboKA9UH_Fzbw+zJax1WPxgtme=kfvp-8chG6w@mail.gmail.com>
 <CAD21AoC0zZqxHPb44akZg2+SzkUrrG23p01NvK=Binkc67sCeg@mail.gmail.com> <CAA4eK1KG9C5vkjQSVi-omT1QJsDoZDnBEiY+rVfNQbqKz0UtYw@mail.gmail.com>
In-Reply-To: <CAA4eK1KG9C5vkjQSVi-omT1QJsDoZDnBEiY+rVfNQbqKz0UtYw@mail.gmail.com>
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Wed, 19 Jul 2023 10:08:38 +0530
Message-ID: <CAExHW5uiKkfW8b6uLuAqjsiiGLTeU2ZRpPkPfzhwrpUmHT3SAA@mail.gmail.com>
Subject: Re: logicalrep_message_type throws an error
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, 
	Euler Taveira <euler@eulerto.com>, pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAExHW5uiKkfW8b6uLuAqjsiiGLTeU2ZRpPkPfzhwrpUmHT3SAA%40mail.gmail.com>
Precedence: bulk

On Wed, Jul 19, 2023 at 9:01=E2=80=AFAM Amit Kapila <amit.kapila16@gmail.co=
m> wrote:
>
> On Tue, Jul 18, 2023 at 10:27=E2=80=AFAM Masahiko Sawada <sawada.mshk@gma=
il.com> wrote:
> >
> > On Tue, Jul 18, 2023 at 12:15=E2=80=AFPM Amit Kapila <amit.kapila16@gma=
il.com> wrote:
> > >
> > > On Mon, Jul 17, 2023 at 7:54=E2=80=AFPM Alvaro Herrera <alvherre@alvh=
.no-ip.org> wrote:
> > > >
> > >
> > > I have tried to check whether we have such usage in any other error
> > > callbacks. Though I haven't scrutinized each and every error callback=
,
> > > I found a few of them where an error can be raised. For example,
> > >
> > > rm_redo_error_callback()->initStringInfo()
> > > CopyFromErrorCallback()->limit_printout_length()
> > > shared_buffer_write_error_callback()->relpathperm()->relpathbackend()=
->GetRelationPath()->psprintf()
> > >
> > > >  Let's
> > > > just do the thing in the original patch you submitted, to ensure al=
l
> > > > these strings are compile-time constants; that's likely the most ro=
bust.
> > > >
> >
> > Or can we use snprintf() writing "??? (%d)" to a fixed length char[8 +
> > 11] allocated on the stack instead?
> >
>
> In the above size calculation, shouldn't it be 7 + 11 where 7 is for
> (3 (???) + 1 for space + 2 for () + 1 for terminating null char) and
> 11 is for %d? BTW, this avoids dynamic allocation of the err string in
> logicalrep_message_type() but we can't return a locally allocated
> string, so do you think we should change the prototype of the function
> to get this as an argument and then use it both for valid and invalid
> cases?

There are other places in the code which do something similar by using
statically allocated buffers like static char xya[SIZE]. We could do
that here. The caller may decide whether to pstrdup this buffer
further or just use it one time e.g. as an elog or printf argument.

As I said before, we should not even print message type in the error
context because it's unknown. Repeating that twice is useless. That
will need some changes to apply_error_callback() though.
But I am fine with "???" as well.

--=20
Best Wishes,
Ashutosh Bapat