Re: Unlogged tables cleanup

On Thu, Dec 8, 2016 at 6:03 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Michael, your greetings were passed on to me with a request that I
> look at this thread.

Thanks for showing up!

> On Fri, Nov 18, 2016 at 12:33 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> More seriously, if there could be more details regarding that, I would
>>>> think that we could say something like "logging the init fork is
>>>> mandatory in any case to ensure its on-disk presence when at recovery
>>>> replay, even on non-default tablespaces whose base location are
>>>> deleted and re-created from scratch if the WAL record in charge of
>>>> creating this tablespace gets replayed". The problem shows up because
>>>> of tablespaces being deleted at replay at the end... So perhaps this
>>>> makes sense. What do you think?
>>>
>>> Yes, that's about what I think we need to explain.
>>
>> OK, what do you think about the attached then?
>>
>> I came up with something like that for those code paths:
>> - /* Write the page. If archiving/streaming, XLOG it. */
>> + /*
>> + * Write the page and log it unconditionally. This is important
>> + * particularly for indexes created on tablespaces and databases
>> + * whose creation happened after the last redo pointer as recovery
>> + * removes any of their existing content when the corresponding
>> + * create records are replayed.
>> + */
>> I have not been able to use the word "create" less than that. The
>> patch is really repetitive, but I think that we had better mention the
>> need of logging the content in each code path and not touch
>> log_newpage() to keep it a maximum flexible.
>
> In blinsert.c, nbtree.c, etc. how about:
>
> Write the page and log it. It might seem that an immediate sync would
> be sufficient to guarantee that the file exists on disk, but recovery
> itself might remove it while replaying, for example, an
> XLOG_DBASE_CREATE record. Therefore, we need this even when
> wal_level=minimal.

OK, I rewrote a bit the patch as attached. What do you think?

>>> Actually, I'm
>>> wondering if we ought to just switch this over to using the delayChkpt
>>> mechanism instead of going through this complicated song-and-dance.
>>> Incurring an immediate sync to avoid having to WAL-log this is a bit
>>> tenuous, but having to WAL-log it AND do the immediate sync just seems
>>> silly, and I'm actually a bit worried that even with your fix there
>>> might still be a latent bug somewhere. We couldn't switch mechanisms
>>> cleanly in the 9.2 branch (cf.
>>> f21bb9cfb5646e1793dcc9c0ea697bab99afa523) so perhaps we should
>>> back-patch it in the form you have it in already, but it's probably
>>> worth switching over at least in master.
>>
>> Thinking through it... Could we not just rip off the sync phase
>> because there is delayChkpt? It seems to me that what counts is that
>> the commit of the transaction that creates the relation does not get
>> past the redo point. It would matter for read uncommitted transactions
>> but that concept does not exist in PG, and never will. On
>> back-branches it is definitely safer to keep the sync, I am just
>> thinking about a HEAD-only optimization here as you do.
>
> Right (I think). If we set and clear delayChkpt around this work, we
> don't need the immediate sync.

My point is a bit different than what you mean I think: the
transaction creating an unlogged relfilenode would not need to even
set delayChkpt in the empty() routines because other transactions
would not refer to it until this transaction has committed. So I am
arguing about just removing the sync phase.
--
Michael