Re: DSA_ALLOC_NO_OOM doesn't work

On Wed, Feb 14, 2024 at 3:23 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 29/01/2024 14:06, Heikki Linnakangas wrote:
> > If you call dsa_allocate_extended(DSA_ALLOC_NO_OOM), it will still
> > ereport an error if you run out of space (originally reported at [0]).
> >
> > Attached patch adds code to test_dsa.c to demonstrate that:
> >
> > postgres=# select test_dsa_basic();
> > ERROR: could not resize shared memory segment "/PostgreSQL.1312700148"
> > to 1075843072 bytes: No space left on device
> >
> > [0] https://github.com/pgvector/pgvector/issues/434#issuecomment-1912744489

Right, DSA_ALLOC_NO_OOM handles the case where there aren't any more
DSM slots (which used to be more common before we ramped up some
constants) and the case where max_total_segment_size (as self-imposed
limit) would be exceeded, but there is nothing to deal with failure to
allocate at the DSM level, and yeah that just isn't a thing it can do.
Not surprisingly that this observation comes from a Docker user: its
64MB default size limit on the /dev/shm mountpoint breaks parallel
query as discussed on list a few times (see also
https://github.com/docker-library/postgres/issues/416)

This is my mistake, introduced in commit 16be2fd10019 where I failed
to pass that down into DSM code. The only user of DSA_ALLOC_NO_OOM in
core code so far is in dshash.c, where we just re-throw after some
cleanup, commit 4569715b, so you could leak that control object due to
this phenomenon.

> I wrote the attached patch to address this, in a fairly straightforward
> or naive way. The problem was that even though dsa_allocate() had code
> to check the return value of dsm_create(), and return NULL instead of
> ereport(ERROR) if the DSA_ALLOC_NO_OOM was set, dsm_create() does not in
> fact return NULL on OOM. To fix, I added a DSM_CREATE_NO_OOM option to
> dsm_create(), and I also had to punch that through to dsm_impl_op().

Yeah, makes total sense.

> This is a little unpolished, but if we want to backpatch something
> narrow now, this would probably be the right approach.
>
> However, I must say that the dsm_impl_op() interface is absolutely
> insane. It's like someone looked at ioctl() and thought, "hey that's a
> great idea!". It mixes all different operations like creating or
> destroying a DSM segment together into one function call, and the return
> value is just a boolean, even though the function could fail for many
> different reasons, and the callers do in fact care about the reason. In
> a more natural interface, the different operations would have very
> different function signatures.

Yeah. It also manages to channel some of shmat() et al's negative beauty.

Anyway, that leads to this treatment of errnos in your patch:

+ errno = 0;
if (dsm_impl_op(DSM_OP_CREATE, seg->handle, size,
&seg->impl_private,
- &seg->mapped_address, &seg->mapped_size, ERROR))
+ &seg->mapped_address, &seg->mapped_size,
ERROR, impl_flags))
break;
+ if ((flags & DSM_CREATE_NO_OOM) != 0 && (errno == ENOMEM
|| errno == ENOSPC))

... which seems reasonable given the constraints. Another idea might
be to write something different in one of those output parameters to
distinguish out-of-memory, but nothing really seems to fit...

> I think we must refactor that. It might be best to leave this
> DSA_ALLOC_NO_OOM bug unfixed in backpatches, and fix it on top of the
> refactorings on master only. Later, we can backpatch the refactorings
> too if we're comfortable with it; extensions shouldn't be using the
> dsm_impl_op() interface directly.

Yeah, that sounds true. We don't do a good job of nailing down the
public API of PostgreSQL but dsm_impl_op() is a rare case that is very
obviously not intended to be called by anyone else. On the other
hand, if we really want to avoid changing the function prototype on
principle, perhaps we could make a new operation DSM_OP_CREATE_NO_OOM
instead?