|From:||Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>|
|To:||Robert Haas <robertmhaas(at)gmail(dot)com>|
|Cc:||Daniel Gustafsson <daniel(at)yesql(dot)se>, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>|
|Subject:||Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 31.07.2020 23:28, Robert Haas wrote:
> On Tue, Jul 14, 2020 at 1:51 PM Anastasia Lubennikova
> <a(dot)lubennikova(at)postgrespro(dot)ru> wrote:
>> Questions from the first review pass:
>> 1) Do we need XLH_INSERT_ALL_VISIBLE_SET ? IIUC, in the patch it is always
>> implied by XLH_INSERT_ALL_FROZEN_SET.
> I agree that it looks unnecessary to have two separate bits.
>> 2) What does this comment mean? Does XXX refers to the lsn comparison?
>> Since it
>> is definitely necessary to update the VM.
>> + * XXX: This seems entirely unnecessary?
>> + *
>> + * FIXME: Theoretically we should only do this after we've
>> + * modified the heap - but it's safe to do it here I think,
>> + * because this means that the page previously was empty.
>> + */
>> + if (lsn > PageGetLSN(vmpage))
>> + visibilitymap_set(reln, blkno, InvalidBuffer, lsn,
>> + InvalidTransactionId, vmbits);
> I wondered about that too. The comment which precedes it was, I
> believe, originally written by me, and copied here from
> heap_xlog_visible(). But it's not clear very good practice to just
> copy the comment like this. If the same logic applies, the code should
> say that we're doing the same thing here as in heap_xlog_visible() for
> consistency, or some such thing; after all, that's the primary place
> where that happens. But it looks like the XXX might have been added by
> a second person who thought that we didn't need this logic at all, and
> the FIXME by a third person who thought it was in the wrong place, so
> the whole thing is really confusing at this point.
> I'm pretty worried about this, too:
> + * FIXME: setting recptr here is a dirty dirty hack, to prevent
> + * visibilitymap_set() from WAL logging.
> That is indeed a dirty hack, and something needs to be done about it.
> I wonder if it was really all that smart to try to make the
> HEAP2_MULTI_INSERT do this instead of just issuing separate WAL
> records to mark it all-visible afterwards, but I don't see any reason
> why this can't be made to work. It needs substantially more polishing
> than it's had, though, I think.
New version of the patch is in the attachment.
New design is more conservative and simpler:
- pin the visibility map page in advance;
- set PageAllVisible;
- call visibilitymap_set() with its own XlogRecord, just like in other
It allows to remove most of the "hacks" and keep code clean.
The patch passes tests added in previous versions.
I haven't tested performance yet, though. Maybe after tests, I'll bring
some optimizations back.
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
|Next Message||Daniel Gustafsson||2020-08-03 09:31:56||Re: Replication & recovery_min_apply_delay|
|Previous Message||Kyotaro Horiguchi||2020-08-03 09:17:56||Re: Is it worth accepting multiple CRLs?|