On Wed, Nov 16, 2016 at 3:54 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Wed, Nov 16, 2016 at 11:45 AM, Robert Haas <robertmh...@gmail.com> wrote: >> The header comment for heap_create_init_fork() says this: >> >> /* >> * Set up an init fork for an unlogged table so that it can be correctly >> * reinitialized on restart. Since we're going to do an immediate sync, we >> * only need to xlog this if archiving or streaming is enabled. And the >> * immediate sync is required, because otherwise there's no guarantee that >> * this will hit the disk before the next checkpoint moves the redo pointer. >> */ >> >> Your patch causes the code not to match the comment any more. And the >> comment explains why at the time I wrote this code I thought it was OK >> to have the XLogIsNeeded() test in there, so it clearly needs >> updating. > > Indeed I missed this comment block. Please let me suggest the following > instead: > /* > * Set up an init fork for an unlogged table so that it can be correctly > - * reinitialized on restart. Since we're going to do an immediate sync, we > - * only need to xlog this if archiving or streaming is enabled. And the > - * immediate sync is required, because otherwise there's no guarantee that > - * this will hit the disk before the next checkpoint moves the redo pointer. > + * reinitialized on restart. An immediate sync is required even if the > + * page has been logged, because the write did not go through > + * shared_buffers and therefore a concurrent checkpoint may have moved > + * the redo pointer past our xlog record. > */
Hmm. Well, that deletes the comment that's no longer true, but it doesn't replace it with any explanation of why we also need to WAL-log it unconditionally, and I think that explanation is not entirely trivial? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
