On Tue, Jul 4, 2017 at 1:23 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Jul 3, 2017 at 7:40 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> It seems to me that this qualifies as an open item for 10. WAL-logging
> is new for hash tables. Amit, could you add one?
>
Added.
> + use_wal = RelationNeedsWAL(rel) ||
> + (rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED &&
> + forkNum == INIT_FORKNUM);
> In access AMs, empty() routines are always only called for unlogged
> relations, so you could relax that to check for INIT_FORKNUM only.
>
Yeah, but _hash_init() is an exposed API, so I am not sure it is a
good idea to make such an assumption at that level of code. Now, as
there is no current user which wants to use it any other way, we can
make such an assumption, but does it serve any purpose?
> Looking at the patch, I think that you are right to do the logging
> directly in _hash_init() and not separately in hashbuildempty().
> Compared to other relations the init data is more dynamic.
>
> + /*
> + * Force the on-disk state of init forks to always be in sync with the
> + * state in shared buffers. See XLogReadBufferForRedoExtended.
> + */
> + XLogRecGetBlockTag(record, 0, NULL, &forknum, NULL);
> + if (forknum == INIT_FORKNUM)
> + FlushOneBuffer(metabuf);
> I find those forced syncs a bit surprising. The buffer is already
> marked as dirty, so even if there is a concurrent checkpoint they
> would be flushed.
>
What if there is no concurrent checkpoint activity and the server is
shutdown? Remember this replay happens on standby and we will just
try to perform Restartpoint and there is no guarantee that it will
flush the buffers. If the buffers are not flushed at shutdown, then
the Init fork will be empty on restart and the hash index will be
corrupt.
> If recovery comes again here, then they would just
> be replayed. I am unclear why XLogReadBufferForRedoExtended is not
> enough to replay a FPW of that.
>
Where does FPW come into the picture for a replay of metapage or bitmappage?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com