Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

Список
Период
Сортировка
От Anastasia Lubennikova
Тема Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits
Дата
Msg-id bf845990-a443-2986-3521-a1fc96b9cbeb@postgrespro.ru
обсуждение исходный текст
Ответ на Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Robert Haas)
Ответы Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Ibrar Ahmed)
Список pgsql-hackers
Дерево обсуждения
COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
 Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Kuntal Ghosh, )
  Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Simon Riggs, )
   Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Kuntal Ghosh, )
 Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Jeff Janes, )
  Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
 Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Masahiko Sawada, )
  Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
   Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Masahiko Sawada, )
    Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
     Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Masahiko Sawada, )
      Re: Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (David Steele, )
      Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
       Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Darafei Praliaskouski, )
       Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Masahiko Sawada, )
        Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
 Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
  Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Tomas Vondra, )
  Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Andres Freund, )
   Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Alvaro Herrera, )
    Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Andres Freund, )
     Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Andres Freund, )
      Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Alvaro Herrera, )
       Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Andres Freund, )
        Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
       Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Andres Freund, )
        Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
         Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Andres Freund, )
        Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Tom Lane, )
         Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Andres Freund, )
          Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Andres Freund, )
           Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Andres Freund, )
            Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
             Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
              Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Amit Kapila, )
               Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Ibrar Ahmed, )
         Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Darafei "Komяpa" Praliaskouski, )
          Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Andres Freund, )
       Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Ibrar Ahmed, )
        Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Ibrar Ahmed, )
         Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Daniel Gustafsson, )
          Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Anastasia Lubennikova, )
           Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Robert Haas, )
            Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Anastasia Lubennikova, )
             Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Ibrar Ahmed, )
              Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Hamid Akhtar, )
               Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Ibrar Ahmed, )
              Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Alvaro Herrera, )
               Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Anastasia Lubennikova, )
                Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Ibrar Ahmed, )
                 Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Anastasia Lubennikova, )
                  Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Ibrar Ahmed, )
                   Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Anastasia Lubennikova, )
                    Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Tatsuo Ishii, )
                     Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Tomas Vondra, )
                      Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Anastasia Lubennikova, )
 Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Tomas Vondra, )
  Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Anastasia Lubennikova, )
   Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Tomas Vondra, )
    Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Anastasia Lubennikova, )
     Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Tomas Vondra, )
      Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Anastasia Lubennikova, )
       Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Tomas Vondra, )
        Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Tomas Vondra, )
         Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Tatsuo Ishii, )
         Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits  (Pavan Deolasee, )
On 31.07.2020 23:28, Robert Haas wrote:
> On Tue, Jul 14, 2020 at 1:51 PM Anastasia Lubennikova
> <> 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,
>> vmbuffer,
>> +                                  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 
places.

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.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Вложения

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Robert Haas
Дата:
Сообщение: Re: Reduce/eliminate the impact of FPW
Следующее
От: Robert Haas
Дата:
Сообщение: Re: Confusing behavior of create table like