Re: GetRelationPath() vs critical sections

Hi,

On 2024-10-06 11:53:59 +0800, Andy Fan wrote:
> Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
>
> > On Thu, Sep 5, 2024 at 3:58 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> >> Obviously we could add a version of GetRelationPath() that just prints into a
> >> caller provided buffer - but that's somewhat awkward API wise.
> >
> > For the record, that's exactly what I did in the patch I proposed to
> > fix our long standing RelationTruncate() data-eating bug:
> >
> > https://www.postgresql.org/message-id/flat/CA%2BhUKG%2B5nfWcpnZ%3DZ%3DUpGvY1tTF%3D4QU_0U_07EFaKmH7Nr%2BNLQ%40mail.gmail.com#aa061db119ee7a4b5390af56e24f475d
>
> I want to have a dicussion on the user provided buffer APIs. I just get
> the similar feedback on [1] because of this recently..
>
> I agree that "user provided buffer" API is bad for the reasons like:
> [agreed to reasons]

I didn't like *any* of the choices, including my own, this thread has
discussed so far. Needing dynamic memory allocation for this seems silly, we
know the max string length ahead of time; introducing new elog.c
infrastructure to handle the dynamic memory allocation is overkill; user
provided buffers seem too error prone; a function returning a pointer into aa
static buffer is not reentrant.

After thinking about this for an embarassingly long time, I think there's
actually a considerably better answer for a case like this: A function that
returns a fixed-length string by value:

- Compilers can fairly easily warn about on-stack values that goes out of
scope

- Because we don't need to free the memory anymore, some code that that
previously needed to explicitly free the memory doesn't need to anymore
(c.f. AbortBufferIO()).

- The max lenght isn't that long, so it's actually reasonably efficient,
likely commonly cheaper than psprintf.

In a preliminary prototype this looks ok, I created a RelPathString struct
containing a char[] of the appropriate length. That seemed less error prone
than a char[] directly, seems that could too easily decay to a pointer or
such.

An accurate length expression is somewhat awkward, but it's also not too
bad. It seems worth going for an accurate length as that makes it easier to
actually verify that length is and stays sufficient.

A hacky sketch for how this could look like is attached.

The naming certainly isn't something to actually use, but I wanted to bring
this up for discussion, before pondering that for even longer.

I converted just a few places to the new interface, there are a lot more that
will look a bit neater (or stop leaking memory, see below).

This made me realize that WaitReadBuffers() leaks a small bit of memory in the
in zero_damaged_pages path. Hardly the end of the world, but it does show how
annoying the current interface is.

Greetings,

Andres Freund