On 21/07/2022 16:23, Yura Sokolov wrote:
> Good day, Thomas
>
> В Пт, 27/05/2022 в 23:24 +1200, Thomas Munro пишет:
>> Rebased, debugged and fleshed out a tiny bit more, but still with
>> plenty of TODO notes and questions. I will talk about this idea at
>> PGCon, so I figured it'd help to have a patch that actually applies,
>> even if it doesn't work quite right yet. It's quite a large patch but
>> that's partly because it removes a lot of lines...
>
> Looks like it have to be rebased again.
Here's a rebase.
I'll write a separate post with my thoughts on the high-level design of
this, but first a couple of more detailed issues:
In RecordTransactionCommit(), we enter a critical section, and then call
TransactionIdCommitTree() to update the CLOG pages. That now involves a
call to ReadBuffer_common(), which in turn calls
ResourceOwnerEnlargeBuffers(). That can fail, because it might require
allocating memory, which is forbidden in a critical section. I ran into
an assertion about that with "make check" when I was playing around with
a heavily modified version of this patch. Haven't seen it with your
original one, but I believe that's just luck.
Calling ResourceOwnerEnlargeBuffers() before entering the critical
section would probably fix that, although I'm a bit worried about having
the Enlarge call so far away from the point where it's needed.
> +void
> +CheckPointSLRU(void)
> +{
> + /* Ensure that directory entries for new files are on disk. */
> + for (int i = 0; i < lengthof(defs); ++i)
> + {
> + if (defs[i].synchronize)
> + fsync_fname(defs[i].path, true);
> + }
> +}
> +
Is it really necessary to fsync() the directories? We don't do that
today for the SLRUs, and we don't do it for the tablespace/db
directories holding relations.
- Heikki
