Horton

Thread

Commits

  1. Change _mdfd_segpath() to return paths by value

  2. Change relpath() et al to return path by value

  1. GetRelationPath() vs critical sections

    Andres Freund <andres@anarazel.de> — 2024-09-04T15:58:33Z

    Hi,

In the AIO patchset there are cases where we have to LOG failures inside a
critical section. This is necessary because e.g. a buffer read might complete
while we are waiting for a WAL write inside a critical section.

We can't just defer the log message, as the IO might end up being
waited-on/completed-by another backend than the backend that issued the IO, so
we'd defer logging issues until an effectively arbitrary later time.

In general emitting a LOG inside a critical section isn't a huge issue - we
made sure that elog.c has a reserve of memory to be able to log without
crashing.

However, the current message for buffer IO issues use relpath*() (ending up in
a call to GetRelationPath()). Which in turn uses psprintf() to generate the
path. Which in turn violates the no-memory-allocations-in-critical-sections
rule, as the containing memory context will typically not have
->allowInCritSection == true.

It's not obvious to me what the best way to deal with this is.

One idea I had was to add an errrelpath() that switches to
edata->assoc_context before calling relpath(), but that would end up leaking
memory, as FreeErrorDataContents() wouldn't know about the allocation.

Obviously we could add a version of GetRelationPath() that just prints into a
caller provided buffer - but that's somewhat awkward API wise.

A third approach would be to have a dedicated memory context for this kind of
thing that's reset after logging the message - but that comes awkwardly close
to duplicating ErrorContext.


I wonder if we're lacking a bit of infrastructure here...

Greetings,

Andres Freund

  2. Re: GetRelationPath() vs critical sections

    Noah Misch <noah@leadboat.com> — 2024-09-04T19:03:53Z

    On Wed, Sep 04, 2024 at 11:58:33AM -0400, Andres Freund wrote:
> In general emitting a LOG inside a critical section isn't a huge issue - we
> made sure that elog.c has a reserve of memory to be able to log without
> crashing.
> 
> However, the current message for buffer IO issues use relpath*() (ending up in
> a call to GetRelationPath()). Which in turn uses psprintf() to generate the
> path. Which in turn violates the no-memory-allocations-in-critical-sections
> rule, as the containing memory context will typically not have
> ->allowInCritSection == true.
> 
> It's not obvious to me what the best way to deal with this is.
> 
> One idea I had was to add an errrelpath() that switches to
> edata->assoc_context before calling relpath(), but that would end up leaking
> memory, as FreeErrorDataContents() wouldn't know about the allocation.
> 
> Obviously we could add a version of GetRelationPath() that just prints into a
> caller provided buffer - but that's somewhat awkward API wise.

Agreed.

> A third approach would be to have a dedicated memory context for this kind of
> thing that's reset after logging the message - but that comes awkwardly close
> to duplicating ErrorContext.

That's how I'd try to do it.  Today's ErrorContext is the context for
allocations FreeErrorDataContents() knows how to find.  The new context would
be for calling into arbitrary code unknown to FreeErrorDataContents().  Most
of the time, we'd avoid reset work for the new context, since it would have no
allocations.

Ideally, errstart() would switch to the new context before returning true, and
errfinish() would switch back.  That way, you could just call relpath*()
without an errrelpath() to help.  This does need functions called in ereport()
argument lists to not use CurrentMemoryContext for allocations that need to
survive longer.  I'd not be concerned about imposing that in a major release.
What obstacles would arise if we did that?

> I wonder if we're lacking a bit of infrastructure here...

Conceptually, the ereport() argument list should be a closure that runs in a
suitable mcxt.  I think we're not far from the goal.

  3. Re: GetRelationPath() vs critical sections

    Thomas Munro <thomas.munro@gmail.com> — 2024-09-04T20:46:57Z

    On Thu, Sep 5, 2024 at 3:58 AM Andres Freund <andres@anarazel.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

  4. Re: GetRelationPath() vs critical sections

    Andy Fan <zhihuifan1213@163.com> — 2024-10-06T03:53:59Z

    Thomas Munro <thomas.munro@gmail.com> writes:

> On Thu, Sep 5, 2024 at 3:58 AM Andres Freund <andres@anarazel.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:
a). inconvenient since user need to provide the buffer. b) unsafe
because user may provide a incorrect buffer. But it still have some
advantages, like c). allocate the memory in a expected MemoryContext
rather than CurrentMemoryContext. d). Allocating the memory at the
different time rather than executing the API e). API can write the data
to the user descired buffer directly rather than another 
copy after. My user case at [1] is because of (c) and (e), and the user
case here looks because of factor (d).

Come to the badness of "user provided buffer" API, I think we can ease
them by providing both the non-user-buffer API and user-provided-buffer
API. Since the later one is safe and convenient, so  
people probably user the non-user-buffer API by default and just the
user who wants the benefits of "provided-buffer" would use that API.

Am I miss some important factors on this topic?

[1]
https://www.postgresql.org/message-id/1882669.1726701697%40sss.pgh.pa.us

(I read the above topic [1] now, I just realize I proposed to [change] the
API rather than adding an new variant, that's not my intention and
that's my fault).

-- 
Best Regards
Andy Fan

  5. Re: GetRelationPath() vs critical sections

    Andres Freund <andres@anarazel.de> — 2025-02-19T02:35:56Z

    Hi,

On 2024-10-06 11:53:59 +0800, Andy Fan wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
>
> > On Thu, Sep 5, 2024 at 3:58 AM Andres Freund <andres@anarazel.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

  6. Re: GetRelationPath() vs critical sections

    Thomas Munro <thomas.munro@gmail.com> — 2025-02-20T01:00:10Z

    On Wed, Feb 19, 2025 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:
> 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.

I like it!

> 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.

Right.  It uses relpath() for an error message, but unlike similar
examples this one is only a WARNING so it might run an unbounded
number of times before the context cleans the memory up.  Not new
code, just moved around in v17.

  7. Re: GetRelationPath() vs critical sections

    Andres Freund <andres@anarazel.de> — 2025-02-20T17:40:57Z

    Hi,

On 2025-02-20 14:00:10 +1300, Thomas Munro wrote:
> On Wed, Feb 19, 2025 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:
> > 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.
> 
> I like it!

Does anybody have opinions about whether we should keep a backward compatible
interface in place or not?

Via https://codesearch.debian.net/ I tried to look for
references.

Unfortunately I had to exclude "relpath" as there are just too many
independent hits, due to the python function of the same name. For
relpathperm(), relpathbackend(), GetRelationPath() there looks to be just
fincore.  There also are two copies of our code, but those we don't need to
care about (libpg-query and ruby-pg-query), since they're just going to copy
the new code when updating.

I also looked for matches including relpath that had "fork" elsewhere in the
file, it's hard to see a potential use of relpath() not using fork in the
arguments or such.

Which makes me think it's not worth having a backward compatible interface?

Greetings,

Andres Freund

  8. Re: GetRelationPath() vs critical sections

    Noah Misch <noah@leadboat.com> — 2025-02-20T19:03:15Z

    On Thu, Feb 20, 2025 at 12:40:57PM -0500, Andres Freund wrote:
> On 2025-02-20 14:00:10 +1300, Thomas Munro wrote:
> > On Wed, Feb 19, 2025 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:
> > > 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.
> > 
> > I like it!

Works for me.

> Unfortunately I had to exclude "relpath" as there are just too many
> independent hits, due to the python function of the same name. For
> relpathperm(), relpathbackend(), GetRelationPath() there looks to be just
> fincore.

PGXN has few hits, and some of these are false positives or otherwise
irrelevant:

$ grep -re '[^.]\(relpath[a-z]*\|GetRelationPath\)(' | sed 's/-[^:]*/:/'|sort -u
db2_fdw::extern char *GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
jsoncdc::    pub fn GetRelationPath(dbNode: Oid, spcNode: Oid, relNode: Oid,
openbarter::        path = relpath(frame.f_code.co_filename, refdir)  # relative to refdir
pg_bulkload::#define relpath(rnode, forknum)            relpath((rnode))
pg_bulkload::   fname = relpath(bknode, MAIN_FORKNUM);
pg_bulkload::   fname = relpath(rnode, MAIN_FORKNUM);
pg_repack::#define relpath(rnode, forknum)              relpath((rnode))
plv8::  def _to_relpath(self, abspath, _):
plv8::  def _to_relpath(self, abspath, test_root):
plv8::          yield self._to_relpath(abspath, test_root)
tblsize_nolock::        relationpath = relpath(*rfn);

> Which makes me think it's not worth having a backward compatible interface?

Agreed.  Even if 100% of those matches had to change, that's below standard
level of breakage for a major release.

  9. Re: GetRelationPath() vs critical sections

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-20T19:11:16Z

    Andres Freund <andres@anarazel.de> writes:
> Does anybody have opinions about whether we should keep a backward compatible
> interface in place or not?

I vote for "not" --- doesn't seem like there'll be much external
code affected, and we make comparably-sized API breaks all the time.

As a matter of style, I wonder if it'd be better to have these
functions write into a caller-supplied variable.  That seems more
in keeping with most other places in Postgres, and it would save
a copying step in cases where the caller needs the result on the
heap.  I realize that returning structs has been in C for decades,
but that doesn't mean I want some of our APIs doing it one way and
some the other.

			regards, tom lane

  10. Re: GetRelationPath() vs critical sections

    Andres Freund <andres@anarazel.de> — 2025-02-20T20:28:39Z

    Hi,

On 2025-02-20 14:11:16 -0500, Tom Lane wrote:
> As a matter of style, I wonder if it'd be better to have these
> functions write into a caller-supplied variable.

I wondered about that too, it does however make some code more awkward,
e.g. because there's not a good surrounding block to put the variable in.

I mildly prefer the return-by-value approach, but not more.


> That seems more in keeping with most other places in Postgres, and it would
> save a copying step in cases where the caller needs the result on the heap.

I don't think there are many cases where we have to then copy the value to the
heap, fwiw, with a more complete patch.


It's perhaps worth noting that on most (all?) architectures the *caller* will
reserve space for the return value of functions that return structs by value.


In general it'll be easier for the compiler to optimize code where it knows
the lifetime of the relevant memory, which is harder for a function that gets
passed a "target" memory location than when it's returning by value. But of
course it's very hard to believe this ever matters for the case at hand.


The patch curently uses a hardcoded 6 for the length of MAX_BACKENDS. Does
anybody have a good idea for how to either

a) derive 6 from MAX_BACKENDS in a way that can be used in a C array size

b) Use a static assert to check that it fits?

Otherwise I'll just put it in the test that my prior version already
added. Not as good as compile-time, but should still make it easy to find the
issue, if we ever increase/decrease MAX_BACKENDS.


In the attached I

1) changed all the GetRelationPath()/relpath* uses to the new way, the names
   now are unchanged
2) introduced a PROCNUMBER_CHARS, instead hardcoding 6 in the length
3) renamed RelPathString to RelPathStr and change the path member to str

I also added a second patch changing _mdfd_segpath() to do something similar,
because it felt somewhat silly to allocate memory after the prior change. Not
entirely sure it's worth it.

Greetings,

Andres Freund

  11. Re: GetRelationPath() vs critical sections

    Thomas Munro <thomas.munro@gmail.com> — 2025-02-21T05:20:39Z

    On Fri, Feb 21, 2025 at 9:28 AM Andres Freund <andres@anarazel.de> wrote:
> The patch curently uses a hardcoded 6 for the length of MAX_BACKENDS. Does
> anybody have a good idea for how to either
>
> a) derive 6 from MAX_BACKENDS in a way that can be used in a C array size

Do we even check the binary digits?  BTW I see a place in lwlock.c
that still talks about 2^23-1, looks like it is out of date.  Hmmm, I
wonder if it would be better to start by declaring how many bits we
want to use, given that is our real constraint.  And then:

#define PROCNUMBER_BITS 18
#define MAX_BACKENDS ((1 << PROCNUMBER_BITS) - 1)
#define PROCNUMBER_CHARS DECIMAL_DIGITS_FOR_BITS(PROCNUMBER_BITS)

... with a little helper ported to preprocessor hell from Hacker's
Delight magic[1] for that last bit.  See attached.  But if that's a
bit too nuts...

> b) Use a static assert to check that it fits?

Right, easy stuff like sizeof(CppString2(MAX_BACKENDS)) - 1 can only
work if the token is a decimal number.  I suppose you could just use
constants:

#define MAX_BACKENDS 0x3ffff
#define PROCNUMBER_BITS 18
#define PROCNUMBER_CHARS 6

... and then use the previous magic to statically assert their
relationship inside one translation unit, to keep it out of a widely
included header.

[1] https://lemire.me/blog/2021/06/03/computing-the-number-of-digits-of-an-integer-even-faster/

  12. Re: GetRelationPath() vs critical sections

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-21T05:35:14Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Fri, Feb 21, 2025 at 9:28 AM Andres Freund <andres@anarazel.de> wrote:
>> The patch curently uses a hardcoded 6 for the length of MAX_BACKENDS. Does
>> anybody have a good idea for how to either
>> 
>> a) derive 6 from MAX_BACKENDS in a way that can be used in a C array size

This all seems quite over-the-top to me.  Just allocate 10 characters,
which is certainly enough for the output of a %u format spec, and
call it good.

(Yeah, I know that's not as much fun, but ...)

			regards, tom lane

  13. Re: GetRelationPath() vs critical sections

    Thomas Munro <thomas.munro@gmail.com> — 2025-02-21T05:35:34Z

    On Fri, Feb 21, 2025 at 6:20 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> #define PROCNUMBER_BITS 18
> #define MAX_BACKENDS ((1 << PROCNUMBER_BITS) - 1)
> #define PROCNUMBER_CHARS DECIMAL_DIGITS_FOR_BITS(PROCNUMBER_BITS)
>
> ... with a little helper ported to preprocessor hell from Hacker's
> Delight magic[1] for that last bit.  See attached.  But if that's a
> bit too nuts...

Erm, wait of course you just need:

#define DECIMAL_DIGITS(n) \
   ((n) < 10 ? 1 : \
    (n) < 100 ? 2 : \
    (n) < 1000 ? 3 : \
    (n) < 10000 ? 4 : \
    (n) < 100000 ? 5 : \
    (n) < 1000000 ? 6 : \
    (n) < 10000000 ? 7 : \
    (n) < 100000000 ? 8 : \
    (n) < 1000000000 ? 9 : 10)

#define PROCNUMBER_BITS 18
#define MAX_BACKENDS ((1 << PROCNUMBER_BITS) - 1)
#define PROCNUMBER_CHARS DECIMAL_DIGITS(MAX_BACKENDS)

  14. Re: GetRelationPath() vs critical sections

    Andres Freund <andres@anarazel.de> — 2025-02-21T16:15:50Z

    Hi,

On 2025-02-21 18:20:39 +1300, Thomas Munro wrote:
> On Fri, Feb 21, 2025 at 9:28 AM Andres Freund <andres@anarazel.de> wrote:
> > The patch curently uses a hardcoded 6 for the length of MAX_BACKENDS. Does
> > anybody have a good idea for how to either
> >
> > a) derive 6 from MAX_BACKENDS in a way that can be used in a C array size
> 
> Do we even check the binary digits?  BTW I see a place in lwlock.c
> that still talks about 2^23-1, looks like it is out of date.

Yea. I actually posted a patch addressing that recently-ish:
https://postgr.es/m/7bye3hvkdk7ybt4ofigimr4ahshsj657aqatfc5trd5pbp5qd4%40dir2wsx2lgo6

Patch 0001 in that series would be nice to have here, because it moves
MAX_BACKENDS out of postmaster.h.  I kind had hoped somebody would comment on
whether pg_config_manual.h is a good place for the define.

I guess we could also move it to storage/procnumber.h.



> Hmmm, I wonder if it would be better to start by declaring how many bits we
> want to use, given that is our real constraint.  And then:
> 
> #define PROCNUMBER_BITS 18
> #define MAX_BACKENDS ((1 << PROCNUMBER_BITS) - 1)
> #define PROCNUMBER_CHARS DECIMAL_DIGITS_FOR_BITS(PROCNUMBER_BITS)
> 
> ... with a little helper ported to preprocessor hell from Hacker's
> Delight magic[1] for that last bit.  See attached.  But if that's a
> bit too nuts...

This seems a bit too complicated to be worth it. I am inclined to think the
approach taken in the patch of just having a regression test verifying that
the numbers match is good enough.

The one arguments I can see for something like DECIMAL_DIGITS_TABLE is that we
could define OIDCHARS the same way. With a quick grep I couldn't immediately
find other candidates though.


> > b) Use a static assert to check that it fits?
> 
> Right, easy stuff like sizeof(CppString2(MAX_BACKENDS)) - 1 can only
> work if the token is a decimal number.  I suppose you could just use
> constants:
> 
> #define MAX_BACKENDS 0x3ffff
> #define PROCNUMBER_BITS 18
> #define PROCNUMBER_CHARS 6
> 
> ... and then use the previous magic to statically assert their
> relationship inside one translation unit, to keep it out of a widely
> included header.

I think we could also just define MAX_BACKENDS as a decimal number. I don't
find 0x3ffff that meaningfully better than 262143

Greetings,

Andres Freund

  15. Re: GetRelationPath() vs critical sections

    Thomas Munro <thomas.munro@gmail.com> — 2025-02-22T00:30:17Z

    On Sat, Feb 22, 2025 at 5:15 AM Andres Freund <andres@anarazel.de> wrote:
> On 2025-02-21 18:20:39 +1300, Thomas Munro wrote:
> > On Fri, Feb 21, 2025 at 9:28 AM Andres Freund <andres@anarazel.de> wrote:
> > Do we even check the binary digits?  BTW I see a place in lwlock.c
> > that still talks about 2^23-1, looks like it is out of date.
>
> Yea. I actually posted a patch addressing that recently-ish:
> https://postgr.es/m/7bye3hvkdk7ybt4ofigimr4ahshsj657aqatfc5trd5pbp5qd4%40dir2wsx2lgo6
>
> Patch 0001 in that series would be nice to have here, because it moves
> MAX_BACKENDS out of postmaster.h.  I kind had hoped somebody would comment on
> whether pg_config_manual.h is a good place for the define.
>
> I guess we could also move it to storage/procnumber.h.

procnumber.h seems like the right place, at least without a separate
discussion of the ramifications of making it configurable, no?  (I
thought there were ideas about squeezing it down to 16 bits so you
could jam two of 'em into an atomic uint32_t for list headers or
something like that, off-topic here except to say that it seems to
conflict with the idea of making it user-increasable?)

> > Hmmm, I wonder if it would be better to start by declaring how many bits we
> > want to use, given that is our real constraint.  And then:
> >
> > #define PROCNUMBER_BITS 18
> > #define MAX_BACKENDS ((1 << PROCNUMBER_BITS) - 1)
> > #define PROCNUMBER_CHARS DECIMAL_DIGITS_FOR_BITS(PROCNUMBER_BITS)
> >
> > ... with a little helper ported to preprocessor hell from Hacker's
> > Delight magic[1] for that last bit.  See attached.  But if that's a
> > bit too nuts...
>
> This seems a bit too complicated to be worth it. I am inclined to think the
> approach taken in the patch of just having a regression test verifying that
> the numbers match is good enough.
>
> The one arguments I can see for something like DECIMAL_DIGITS_TABLE is that we
> could define OIDCHARS the same way. With a quick grep I couldn't immediately
> find other candidates though.

If anyone ever does consider using something like that, whether to
derive values or just to make assertions that they agree inside a
translation unit somewhere, FTR here's why I posted the simple version
after being temporarily nerdsniped by Hacker's Delight: the
bitswizzling algorithm was translated from branch-free code, which is
fun but irrelevant here and that property didn't even survive the
translation.  So you might as well just use the naive definition of
DECIMAL_DIGITS(n) I posted immediately after.

  16. Re: GetRelationPath() vs critical sections

    Thomas Munro <thomas.munro@gmail.com> — 2025-02-22T00:42:35Z

    On Sat, Feb 22, 2025 at 1:30 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> procnumber.h seems like the right place, at least without a separate
> discussion of the ramifications of making it configurable, no?  (I
> thought there were ideas about squeezing it down to 16 bits so you
> could jam two of 'em into an atomic uint32_t for list headers or
> something like that, off-topic here except to say that it seems to
> conflict with the idea of making it user-increasable?)

(Please ignore this comment, I'll comment on the other thread instead.
Sorry I hadn't seen your patches over there yet: when I started
talking about the definition and assertions around MAX_BACKENDS in
here, it had jumped out at me independently while trying to answer
your question about compile-time log10 stuff, because I also noticed
that we sucked even at codifying the log2 constraints.)

  17. Re: GetRelationPath() vs critical sections

    Andres Freund <andres@anarazel.de> — 2025-02-24T15:50:21Z

    Hi,

On 2025-02-20 15:28:39 -0500, Andres Freund wrote:
> On 2025-02-20 14:11:16 -0500, Tom Lane wrote:
> > As a matter of style, I wonder if it'd be better to have these
> > functions write into a caller-supplied variable.
> 
> I wondered about that too, it does however make some code more awkward,
> e.g. because there's not a good surrounding block to put the variable in.
> 
> I mildly prefer the return-by-value approach, but not more.

Since I've not heard anything on that point, I'm currently thinking with going
with return-by-value. Attached are two very mildly updated patches. Most of
the changes is added commit messages.

Greetings,

Andres Freund

  18. Re: GetRelationPath() vs critical sections

    Andres Freund <andres@anarazel.de> — 2025-02-25T14:24:00Z

    Hi,

On 2025-02-24 10:50:21 -0500, Andres Freund wrote:
> Since I've not heard anything on that point, I'm currently thinking with going
> with return-by-value. Attached are two very mildly updated patches. Most of
> the changes is added commit messages.

And pushed.

Greetings,

Andres Freund