On Mon, Feb 13, 2017 at 10:22 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > As discussed, attached are refactoring patches and a patch to enable > WAL for the hash index on top of them.
Thanks. I think that the refactoring patches shouldn't add START_CRIT_SECTION() and END_CRIT_SECTION() calls; let's leave that for the final patch. Nor should their comments reference the idea of writing WAL; that should also be for the final patch. PageGetFreeSpaceForMulTups doesn't seem like a great name. PageGetFreeSpaceForMultipleTuples? Or maybe just use PageGetExactFreeSpace and then do the account in the caller. I'm not sure it's really worth having a function just to subtract a multiple of sizeof(ItemIdData), and it would actually be more efficient to have the caller take care of this, since you wouldn't need to keep recalculating the value for every iteration of the loop. I think we ought to just rip (as a preliminary patch) out the support for freeing an overflow page in the middle of a bucket chain. That code is impossible to hit, I believe, and instead of complicating the WAL machinery to cater to a nonexistent case, maybe we ought to just get rid of it. + /* + * We need to release and if required reacquire the lock on + * rbuf to ensure that standby shouldn't see an intermediate + * state of it. + */ How does releasing and reacquiring the lock on the master affect whether the standby can see an intermediate state? That sounds totally wrong to me (and also doesn't belong in a refactoring patch anyway since we're not emitting any WAL here). - Assert(PageIsNew(page)); This worries me a bit. What's going on here? -- 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