From: | Michail Nikolaev <michail(dot)nikolaev(at)gmail(dot)com> |
---|---|
To: | Antonin Houska <ah(at)cybertec(dot)at> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [PATCH] Full support for index LP_DEAD hint bits on standby |
Date: | 2021-11-05 16:51:32 |
Message-ID: | CANtu0oiEOnJYjF6x-Z2ZQDSctq_XPmbd6n8228PzJiS9oR+-_Q@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello, Antonin.
Thanks for pushing it forward.
> I understand that the RR snapshot is used to check the MVCC behaviour, however
> this comment seems to indicate that the RR snapshot should also prevent the
> standb from setting the hint bits.
> # Make sure previous queries not set the hints on standby because
> # of RR snapshot
> I can imagine that on the primary, but I don't think that the backend that
> checks visibility on standby does checks other snapshots/backends. And it
> didn't work when I ran the test manually, although I could have missed
> something.
Yes, it checks - you could see ComputeXidHorizons for details. It is
the main part of the correctness of the whole feature. I added some
details about it to the test.
> * 026_standby_index_lp_dead.pl should probably be renamed to
> 027_standby_index_lp_dead.pl (026_* was created in the master branch
> recently)
Done.
> BEGIN failed--compilation aborted at t/026_standby_index_lp_dead.pl line 5.
> t/026_standby_index_lp_dead.pl .. Dubious, test returned 2 (wstat 512, 0x200)
Fixed.
> * The messages like this
Fixed.
> * There's an extra colon in mask_lp_dead():
Oh, it is a huge error really (the loop was empty) :) Fixed.
> * the header comment of heap_hot_search_buffer() still says "*all_dead"
> whereas I'd expect "->all_dead".
> The same for "*page_lsn".
I was trying to mimic the style of comment (it says about “*tid” from
2007). So, I think it is better to keep it in the same style for the
whole function comment.
> * I can see no test for the INDEX_LP_DEAD_OK_MIN_LSN value of the
> IndexLpDeadAllowedResult enumeration. Shouldn't there be only two values,
> e.g. INDEX_LP_DEAD_OK and INDEX_LP_DEAD_MAYBE_OK ? Or a boolean variable (in
> index_fetch_heap()) of the appropriate name, e.g. kill_maybe_allowed, and
> rename the function is_index_lp_dead_allowed() to
> is_index_lp_dead_maybe_allowed()?
Yes, this way it is looks better. Done. Also, I have added some checks
for “maybe” LSN-related logic to the test.
Thanks a lot,
Michail.
Attachment | Content-Type | Size |
---|---|---|
v5-0003-doc.patch | text/x-patch | 4.3 KB |
v5-0001-code.patch | text/x-patch | 51.2 KB |
v5-0002-test.patch | text/x-patch | 13.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2021-11-05 16:58:04 | Re: removing global variable ThisTimeLineID |
Previous Message | Andres Freund | 2021-11-05 16:48:16 | Re: WIP: expression evaluation improvements |