Re: logicalrep_message_type throws an error

On Wed, Jul 19, 2023 at 9:01 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Jul 18, 2023 at 10:27 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Tue, Jul 18, 2023 at 12:15 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Jul 17, 2023 at 7:54 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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 all
> > > > these strings are compile-time constants; that's likely the most robust.
> > > >
> >
> > 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.

--
Best Wishes,
Ashutosh Bapat