Re: AIO v2.2 - Mailing list pgsql-hackers

Hi,

On 2025-01-07 18:08:51 +0200, Heikki Linnakangas wrote:
> On LWLockDisown():
> 
> > +/*
> > + * Stop treating lock as held by current backend.
> > + *
> > + * After calling this function it's the callers responsibility to ensure that
> > + * the lock gets released, even in case of an error. This only is desirable if
> > + * the lock is going to be released in a different process than the process
> > + * that acquired it.
> > + *
> > + * Returns the mode in which the lock was held by the current backend.
> 
> Returning the lock mode feels a bit ad hoc..

It seemed useful to me, that way callers could verify that the released lock
level is actually what it expected. What do we gain by hiding this information
anyway?

Orthogonal: I think it was a mistake that LWLockRelease() didn't require the
to-be-releaased lock mode to be passed in...


> > + * NB: This will leave lock->owner pointing to the current backend (if
> > + * LOCK_DEBUG is set). We could add a separate flag indicating that, but it
> > + * doesn't really seem worth it.
> 
> Hmm. I won't insist, but I feel it probably would be worth it. This is only
> in LOCK_DEBUG mode so there's no performance penalty in non-debug builds,
> and when you do compile with LOCK_DEBUG you probably appreciate any extra
> information.

I actually thought it'd be more useful if it stays pointing to the 'original
owner'.

When you say "it" would be worth it, you mean resetting owner, or adding a
flag indicating that it's a disowned lock?



> > + * NB: This does not call RESUME_INTERRUPTS(), but leaves that responsibility
> > + * of the caller.
> > + */
> 
> That feels weird. The only caller outside lwlock.c does call
> RESUME_INTERRUPTS() immediately.

Yea, I didn't feel happy with it either. It just seemed that the cure (a
separate function, or a parameter indicating whether interrupts should be
resumed) was as bad as the disease.


> Perhaps it'd make for a better external interface if LWLockDisown() did call
> RESUME_INTERRUPTS(), and there was a separate internal version that didn't.

Hm, that seems more complicated than it's worth.  I'd either leave it as-is,
or add a parameter to LWLockDisown to indicate if interrupts should be
resumed.


> And it might make more sense for the external version to return 'void' while
> we're at it. Returning a value that the caller ignores is harmless, of
> course, but it feels a bit weird. It makes you wonder what you're supposed
> to do with it.

This one I disagree with, I think it makes a lot of sense to return the lock
mode of the lock you just disowned.

Doubtful it matters, but the compiler can trivially optimize that out for the
lwlock.c callers.


> > +    {
> > +        {"io_method", PGC_POSTMASTER, RESOURCES_MEM,
> > +            gettext_noop("Selects the method of asynchronous I/O to use."),
> > +            NULL
> > +        },
> > +        &io_method,
> > +        DEFAULT_IO_METHOD, io_method_options,
> > +        NULL, assign_io_method, NULL
> > +    },
> > +
> 
> The description is a bit funny because synchronous I/O is one of the
> possible methods.

Hah.  How about:

"Selects the method of, potentially asynchronous, IO execution."?

Greetings,

Andres Freund