Setting pd_lower in GIN metapage
What are some arguments against setting pd_lower in the GIN metapage as
follows?
GinMetaPageData *metad = GinPageGetMeta(page);
((PageHeader) page)->pd_lower =
((char *) metad + sizeof(GinMetaPageData)) - (char *) page;
I saw that _bt_initmetapage() does it, so was wondering why doesn't GIN.
How about porting such a change to the back-branches if we do this at all?
I couldn't find any discussion in the archives about this. I read in
comments that server versions older than 9.4 didn't set pd_lower correctly
in any of GIN index pages, so relying on pd_lower value of GIN pages is
unreliable in general.
The reason I'm asking is that a certain backup tool relies on pd_lower
values of data pages (disk blocks in relation files that are known to have
a valid PageHeaderData) to be correct to discard the portion of every page
that supposedly does not contain any useful information. The assumption
doesn't hold in the case of GIN metapage, so any GIN indexes contain
corrupted metapage after recovery (metadata overwritten with zeros).
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 19, 2017 at 11:37 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
What are some arguments against setting pd_lower in the GIN metapage as
follows?GinMetaPageData *metad = GinPageGetMeta(page);
((PageHeader) page)->pd_lower =
((char *) metad + sizeof(GinMetaPageData)) - (char *) page;I saw that _bt_initmetapage() does it, so was wondering why doesn't GIN.
Actually, hash index also has similar code (See _hash_init_metabuffer)
and I see no harm in doing this at similar other places. This helps
in compressing the hole in metapage during WAL writing. I think that
in itself might not be an argument in favor of doing this because
there is only one meta page for index and saving ~7K WAL is not huge
but OTOH I don't see a reason for not doing it.
How about porting such a change to the back-branches if we do this at all?
I couldn't find any discussion in the archives about this. I read in
comments that server versions older than 9.4 didn't set pd_lower correctly
in any of GIN index pages, so relying on pd_lower value of GIN pages is
unreliable in general.The reason I'm asking is that a certain backup tool relies on pd_lower
values of data pages (disk blocks in relation files that are known to have
a valid PageHeaderData) to be correct to discard the portion of every page
that supposedly does not contain any useful information. The assumption
doesn't hold in the case of GIN metapage, so any GIN indexes contain
corrupted metapage after recovery (metadata overwritten with zeros).
Why can't you do a special check for metapage identification? It
should be easy to add such a check. I have seen one of such tools
(pg_filedump) has similar check to skip metapage in certain code
paths.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Amit Kapila <amit.kapila16@gmail.com> writes:
On Mon, Jun 19, 2017 at 11:37 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:What are some arguments against setting pd_lower in the GIN metapage as
follows?
Actually, hash index also has similar code (See _hash_init_metabuffer)
and I see no harm in doing this at similar other places.
Seems reasonable.
How about porting such a change to the back-branches if we do this at all?
The reason I'm asking is that a certain backup tool relies on pd_lower
values of data pages (disk blocks in relation files that are known to have
a valid PageHeaderData) to be correct to discard the portion of every page
that supposedly does not contain any useful information. The assumption
doesn't hold in the case of GIN metapage, so any GIN indexes contain
corrupted metapage after recovery (metadata overwritten with zeros).
I'm not in favor of back-porting such a change. Even if we did, it would
only affect subsequently-created indexes not existing ones. That means
your tool has to cope with an unset pd_lower in any case --- and will for
the foreseeable future, because of pg_upgrade.
I'd suggest a rule like "if pd_lower is smaller than SizeOfPageHeaderData
then don't trust it, but assume all of the page is valid data".
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/06/19 22:59, Amit Kapila wrote:
On Mon, Jun 19, 2017 at 11:37 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:What are some arguments against setting pd_lower in the GIN metapage as
follows?GinMetaPageData *metad = GinPageGetMeta(page);
((PageHeader) page)->pd_lower =
((char *) metad + sizeof(GinMetaPageData)) - (char *) page;I saw that _bt_initmetapage() does it, so was wondering why doesn't GIN.
Actually, hash index also has similar code (See _hash_init_metabuffer)
and I see no harm in doing this at similar other places. This helps
in compressing the hole in metapage during WAL writing. I think that
in itself might not be an argument in favor of doing this because
there is only one meta page for index and saving ~7K WAL is not huge
but OTOH I don't see a reason for not doing it.
I agree. Will write a patch.
How about porting such a change to the back-branches if we do this at all?
I couldn't find any discussion in the archives about this. I read in
comments that server versions older than 9.4 didn't set pd_lower correctly
in any of GIN index pages, so relying on pd_lower value of GIN pages is
unreliable in general.The reason I'm asking is that a certain backup tool relies on pd_lower
values of data pages (disk blocks in relation files that are known to have
a valid PageHeaderData) to be correct to discard the portion of every page
that supposedly does not contain any useful information. The assumption
doesn't hold in the case of GIN metapage, so any GIN indexes contain
corrupted metapage after recovery (metadata overwritten with zeros).Why can't you do a special check for metapage identification? It
should be easy to add such a check. I have seen one of such tools
(pg_filedump) has similar check to skip metapage in certain code
paths.
I tried to add a metapage check, but getting such code to compile requires
it to include headers like gin_private.h (in PG versions < 10), which in
turn includes other headers that are forbidden to be included in what's
supposed to be FRONTEND code. (thread at [1]/messages/by-id/CA+TgmoZ=F=GkxV0YEv-A8tb+AEGy_Qa7GSiJ8deBKFATnzfEug@mail.gmail.com seems relevant in this
regard.) Another way to fix this might be to copy the GinMetaPageData
struct definition and a few other symbols into the tool's code and make
necessary checks using the same, instead of including gin_private.h.
Thanks,
Amit
[1]: /messages/by-id/CA+TgmoZ=F=GkxV0YEv-A8tb+AEGy_Qa7GSiJ8deBKFATnzfEug@mail.gmail.com
/messages/by-id/CA+TgmoZ=F=GkxV0YEv-A8tb+AEGy_Qa7GSiJ8deBKFATnzfEug@mail.gmail.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/06/19 23:31, Tom Lane wrote:
Amit Kapila <amit.kapila16@gmail.com> writes:
On Mon, Jun 19, 2017 at 11:37 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:What are some arguments against setting pd_lower in the GIN metapage as
follows?Actually, hash index also has similar code (See _hash_init_metabuffer)
and I see no harm in doing this at similar other places.Seems reasonable.
Here is a patch that does it for the GIN metapage. (I am not sure if the
changes to gin_mask() that are included in the patch are really necessary.)
How about porting such a change to the back-branches if we do this at all?
The reason I'm asking is that a certain backup tool relies on pd_lower
values of data pages (disk blocks in relation files that are known to have
a valid PageHeaderData) to be correct to discard the portion of every page
that supposedly does not contain any useful information. The assumption
doesn't hold in the case of GIN metapage, so any GIN indexes contain
corrupted metapage after recovery (metadata overwritten with zeros).I'm not in favor of back-porting such a change. Even if we did, it would
only affect subsequently-created indexes not existing ones. That means
your tool has to cope with an unset pd_lower in any case --- and will for
the foreseeable future, because of pg_upgrade.I'd suggest a rule like "if pd_lower is smaller than SizeOfPageHeaderData
then don't trust it, but assume all of the page is valid data".
Actually, such a check is already in place in the tool, whose condition
looks like:
if (PageGetPageSize(header) == BLCKSZ &&
PageGetPageLayoutVersion(header) == PG_PAGE_LAYOUT_VERSION &&
(header->pd_flags & ~PD_VALID_FLAG_BITS) == 0 &&
header->pd_lower >= SizeOfPageHeaderData &&
header->pd_lower <= header->pd_upper &&
header->pd_upper <= header->pd_special &&
header->pd_special <= BLCKSZ &&
header->pd_special == MAXALIGN(header->pd_special) && ...
which even GIN metapage passes, making it an eligible data page and hence
for omitting the hole between pd_lower and pd_upper.
That's because a GIN metapage will always have undergone PageInit() that
sets pd_lower to SizeOfPageHeaderData. Which means the tool has to look
beyond the standard PageHeaderData to determine whether the area between
pd_lower and pd_upper is really a hole. Amit K also suggested the same,
but that seems to require either duplicating GIN's private struct
definition (of GinMetaPageData) in the tool or including backend's
gin_private.h, either of which doesn't seem to be a good thing to do in
what is FRONTEND code, but maybe there is no other way. Am I missing
something?
Thanks,
Amit
Attachments:
set-gin-metapage-pd_lower-v1.patchtext/plain; charset=UTF-8; name=set-gin-metapage-pd_lower-v1.patchDownload+13-13
On Tue, Jun 20, 2017 at 1:50 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2017/06/19 23:31, Tom Lane wrote:
Amit Kapila <amit.kapila16@gmail.com> writes:
On Mon, Jun 19, 2017 at 11:37 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:What are some arguments against setting pd_lower in the GIN metapage as
follows?Actually, hash index also has similar code (See _hash_init_metabuffer)
and I see no harm in doing this at similar other places.Seems reasonable.
Here is a patch that does it for the GIN metapage. (I am not sure if the
changes to gin_mask() that are included in the patch are really necessary.)How about porting such a change to the back-branches if we do this at all?
The reason I'm asking is that a certain backup tool relies on pd_lower
values of data pages (disk blocks in relation files that are known to have
a valid PageHeaderData) to be correct to discard the portion of every page
that supposedly does not contain any useful information. The assumption
doesn't hold in the case of GIN metapage, so any GIN indexes contain
corrupted metapage after recovery (metadata overwritten with zeros).I'm not in favor of back-porting such a change. Even if we did, it would
only affect subsequently-created indexes not existing ones. That means
your tool has to cope with an unset pd_lower in any case --- and will for
the foreseeable future, because of pg_upgrade.I'd suggest a rule like "if pd_lower is smaller than SizeOfPageHeaderData
then don't trust it, but assume all of the page is valid data".Actually, such a check is already in place in the tool, whose condition
looks like:if (PageGetPageSize(header) == BLCKSZ &&
PageGetPageLayoutVersion(header) == PG_PAGE_LAYOUT_VERSION &&
(header->pd_flags & ~PD_VALID_FLAG_BITS) == 0 &&
header->pd_lower >= SizeOfPageHeaderData &&
header->pd_lower <= header->pd_upper &&
header->pd_upper <= header->pd_special &&
header->pd_special <= BLCKSZ &&
header->pd_special == MAXALIGN(header->pd_special) && ...which even GIN metapage passes, making it an eligible data page and hence
for omitting the hole between pd_lower and pd_upper.
Won't checking for GIN_META in header->pd_flags gives you what you want?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/06/20 20:37, Amit Kapila wrote:
On Tue, Jun 20, 2017 at 1:50 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:On 2017/06/19 23:31, Tom Lane wrote:
I'd suggest a rule like "if pd_lower is smaller than SizeOfPageHeaderData
then don't trust it, but assume all of the page is valid data".Actually, such a check is already in place in the tool, whose condition
looks like:if (PageGetPageSize(header) == BLCKSZ &&
PageGetPageLayoutVersion(header) == PG_PAGE_LAYOUT_VERSION &&
(header->pd_flags & ~PD_VALID_FLAG_BITS) == 0 &&
header->pd_lower >= SizeOfPageHeaderData &&
header->pd_lower <= header->pd_upper &&
header->pd_upper <= header->pd_special &&
header->pd_special <= BLCKSZ &&
header->pd_special == MAXALIGN(header->pd_special) && ...which even GIN metapage passes, making it an eligible data page and hence
for omitting the hole between pd_lower and pd_upper.Won't checking for GIN_META in header->pd_flags gives you what you want?
GIN_META flag is not written into pd_flags but GinPageOpaqueData.flags,
which still requires including GIN's private header.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 21, 2017 at 9:42 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2017/06/20 20:37, Amit Kapila wrote:
On Tue, Jun 20, 2017 at 1:50 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:On 2017/06/19 23:31, Tom Lane wrote:
I'd suggest a rule like "if pd_lower is smaller than SizeOfPageHeaderData
then don't trust it, but assume all of the page is valid data".Actually, such a check is already in place in the tool, whose condition
looks like:if (PageGetPageSize(header) == BLCKSZ &&
PageGetPageLayoutVersion(header) == PG_PAGE_LAYOUT_VERSION &&
(header->pd_flags & ~PD_VALID_FLAG_BITS) == 0 &&
header->pd_lower >= SizeOfPageHeaderData &&
header->pd_lower <= header->pd_upper &&
header->pd_upper <= header->pd_special &&
header->pd_special <= BLCKSZ &&
header->pd_special == MAXALIGN(header->pd_special) && ...which even GIN metapage passes, making it an eligible data page and hence
for omitting the hole between pd_lower and pd_upper.Won't checking for GIN_META in header->pd_flags gives you what you want?
GIN_META flag is not written into pd_flags but GinPageOpaqueData.flags,
which still requires including GIN's private header.
Did you check this patch with wal_consistency_checking? I am getting
failures so your patch does not have the masking of GIN pages
completely right:
FATAL: inconsistent page found, rel 1663/16385/28133, forknum 0, blkno 0
CONTEXT: WAL redo at 0/39379EB8 for Gin/UPDATE_META_PAGE:
That's easily reproducible with installcheck and a standby replaying
the changes. I did not look at the code in details to see what you may
be missing here.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/06/22 16:56, Michael Paquier wrote:
On Wed, Jun 21, 2017 at 9:42 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:On 2017/06/20 20:37, Amit Kapila wrote:
On Tue, Jun 20, 2017 at 1:50 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:On 2017/06/19 23:31, Tom Lane wrote:
I'd suggest a rule like "if pd_lower is smaller than SizeOfPageHeaderData
then don't trust it, but assume all of the page is valid data".Actually, such a check is already in place in the tool, whose condition
looks like:if (PageGetPageSize(header) == BLCKSZ &&
PageGetPageLayoutVersion(header) == PG_PAGE_LAYOUT_VERSION &&
(header->pd_flags & ~PD_VALID_FLAG_BITS) == 0 &&
header->pd_lower >= SizeOfPageHeaderData &&
header->pd_lower <= header->pd_upper &&
header->pd_upper <= header->pd_special &&
header->pd_special <= BLCKSZ &&
header->pd_special == MAXALIGN(header->pd_special) && ...which even GIN metapage passes, making it an eligible data page and hence
for omitting the hole between pd_lower and pd_upper.Won't checking for GIN_META in header->pd_flags gives you what you want?
GIN_META flag is not written into pd_flags but GinPageOpaqueData.flags,
which still requires including GIN's private header.Did you check this patch with wal_consistency_checking? I am getting
failures so your patch does not have the masking of GIN pages
completely right:
FATAL: inconsistent page found, rel 1663/16385/28133, forknum 0, blkno 0
CONTEXT: WAL redo at 0/39379EB8 for Gin/UPDATE_META_PAGE:
That's easily reproducible with installcheck and a standby replaying
the changes. I did not look at the code in details to see what you may
be missing here.
Oh, wasn't sure about the gin_mask() changes myself. Thanks for checking.
Actually, the WAL consistency check fails even without patching
gin_mask(), so the problem may be with the main patch itself. That is,
the patch needs to do something else other than just teaching
GinInitMetabuffer() to initialize pd_lower. Will look into that.
Thanks,
Amit
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 22, 2017 at 6:55 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2017/06/22 16:56, Michael Paquier wrote:
On Wed, Jun 21, 2017 at 9:42 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:On 2017/06/20 20:37, Amit Kapila wrote:
On Tue, Jun 20, 2017 at 1:50 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:On 2017/06/19 23:31, Tom Lane wrote:
I'd suggest a rule like "if pd_lower is smaller than SizeOfPageHeaderData
then don't trust it, but assume all of the page is valid data".Actually, such a check is already in place in the tool, whose condition
looks like:if (PageGetPageSize(header) == BLCKSZ &&
PageGetPageLayoutVersion(header) == PG_PAGE_LAYOUT_VERSION &&
(header->pd_flags & ~PD_VALID_FLAG_BITS) == 0 &&
header->pd_lower >= SizeOfPageHeaderData &&
header->pd_lower <= header->pd_upper &&
header->pd_upper <= header->pd_special &&
header->pd_special <= BLCKSZ &&
header->pd_special == MAXALIGN(header->pd_special) && ...which even GIN metapage passes, making it an eligible data page and hence
for omitting the hole between pd_lower and pd_upper.Won't checking for GIN_META in header->pd_flags gives you what you want?
GIN_META flag is not written into pd_flags but GinPageOpaqueData.flags,
which still requires including GIN's private header.Did you check this patch with wal_consistency_checking? I am getting
failures so your patch does not have the masking of GIN pages
completely right:
FATAL: inconsistent page found, rel 1663/16385/28133, forknum 0, blkno 0
CONTEXT: WAL redo at 0/39379EB8 for Gin/UPDATE_META_PAGE:
That's easily reproducible with installcheck and a standby replaying
the changes. I did not look at the code in details to see what you may
be missing here.Oh, wasn't sure about the gin_mask() changes myself. Thanks for checking.
Actually, the WAL consistency check fails even without patching
gin_mask(), so the problem may be with the main patch itself. That is,
the patch needs to do something else other than just teaching
GinInitMetabuffer() to initialize pd_lower. Will look into that.
I've not read the code deeply but I guess we should use
GinInitMetabuffer() in ginRedoUpdateMetapage() instead of
GinInitPage(). Maybe also GinInitPage() in ginRedoDeleteListPages() is
the same.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/06/23 10:22, Masahiko Sawada wrote:
On Thu, Jun 22, 2017 at 6:55 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:On 2017/06/22 16:56, Michael Paquier wrote:
Did you check this patch with wal_consistency_checking? I am getting
failures so your patch does not have the masking of GIN pages
completely right:
FATAL: inconsistent page found, rel 1663/16385/28133, forknum 0, blkno 0
CONTEXT: WAL redo at 0/39379EB8 for Gin/UPDATE_META_PAGE:
That's easily reproducible with installcheck and a standby replaying
the changes. I did not look at the code in details to see what you may
be missing here.Oh, wasn't sure about the gin_mask() changes myself. Thanks for checking.
Actually, the WAL consistency check fails even without patching
gin_mask(), so the problem may be with the main patch itself. That is,
the patch needs to do something else other than just teaching
GinInitMetabuffer() to initialize pd_lower. Will look into that.I've not read the code deeply but I guess we should use
GinInitMetabuffer() in ginRedoUpdateMetapage() instead of
GinInitPage(). Maybe also GinInitPage() in ginRedoDeleteListPages() is
the same.
That was it, thanks for the pointer.
Attached updated patch, which I confirmed, passes wal_consistency_check = gin.
Thanks,
Amit
Attachments:
set-gin-metapage-pd_lower-v2.patchtext/plain; charset=UTF-8; name=set-gin-metapage-pd_lower-v2.patchDownload+15-15
On Fri, Jun 23, 2017 at 11:17 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2017/06/23 10:22, Masahiko Sawada wrote:
On Thu, Jun 22, 2017 at 6:55 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:On 2017/06/22 16:56, Michael Paquier wrote:
Did you check this patch with wal_consistency_checking? I am getting
failures so your patch does not have the masking of GIN pages
completely right:
FATAL: inconsistent page found, rel 1663/16385/28133, forknum 0, blkno 0
CONTEXT: WAL redo at 0/39379EB8 for Gin/UPDATE_META_PAGE:
That's easily reproducible with installcheck and a standby replaying
the changes. I did not look at the code in details to see what you may
be missing here.Oh, wasn't sure about the gin_mask() changes myself. Thanks for checking.
Actually, the WAL consistency check fails even without patching
gin_mask(), so the problem may be with the main patch itself. That is,
the patch needs to do something else other than just teaching
GinInitMetabuffer() to initialize pd_lower. Will look into that.I've not read the code deeply but I guess we should use
GinInitMetabuffer() in ginRedoUpdateMetapage() instead of
GinInitPage(). Maybe also GinInitPage() in ginRedoDeleteListPages() is
the same.That was it, thanks for the pointer.
Attached updated patch, which I confirmed, passes wal_consistency_check = gin.
Thank you for updating the patch. It looks good to me.
BTW I'm inclined to have a regression test case where doing 'make
check' to the streaming replication environment with
wal_consistency_check on standby server so that we can detect a bug
around the wal.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 23, 2017 at 2:56 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Thank you for updating the patch. It looks good to me.
BTW I'm inclined to have a regression test case where doing 'make
check' to the streaming replication environment with
wal_consistency_check on standby server so that we can detect a bug
around the wal.
This would be very costly. A single run of the main regression tests
generates 15 to 20GB of FPWs if I recall correctly. Tiny buildfarm
members would suffer on that. *cough*
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/06/23 15:07, Michael Paquier wrote:
On Fri, Jun 23, 2017 at 2:56 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Thank you for updating the patch. It looks good to me.
BTW I'm inclined to have a regression test case where doing 'make
check' to the streaming replication environment with
wal_consistency_check on standby server so that we can detect a bug
around the wal.This would be very costly. A single run of the main regression tests
generates 15 to 20GB of FPWs if I recall correctly. Tiny buildfarm
members would suffer on that. *cough*
Initially, I had naively set wal_consistency_check = all before running
make installcheck and then had to wait for a long time to confirm that WAL
generated by the gin test indeed caused consistency check failure on the
standby with the v1 patch.
But I can see Sawada-san's point that there should be some way for
developers writing code that better had gone through WAL consistency
checking facility to do it without much hassle. But then again, it may
not be that frequent to need that.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 23, 2017 at 3:17 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
Initially, I had naively set wal_consistency_check = all before running
make installcheck and then had to wait for a long time to confirm that WAL
generated by the gin test indeed caused consistency check failure on the
standby with the v1 patch.
wal_consistency_check = gin would have saved you a lot of I/O.
But I can see Sawada-san's point that there should be some way for
developers writing code that better had gone through WAL consistency
checking facility to do it without much hassle. But then again, it may
not be that frequent to need that.
Yeah, I have my own set of generic scripts for that. There could be a
set of modules out of the main check-world, the risk that those finish
rotting is high though...
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 23, 2017 at 3:31 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Fri, Jun 23, 2017 at 3:17 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:Initially, I had naively set wal_consistency_check = all before running
make installcheck and then had to wait for a long time to confirm that WAL
generated by the gin test indeed caused consistency check failure on the
standby with the v1 patch.wal_consistency_check = gin would have saved you a lot of I/O.
But I can see Sawada-san's point that there should be some way for
developers writing code that better had gone through WAL consistency
checking facility to do it without much hassle. But then again, it may
not be that frequent to need that.
Yeah, it should be optional. I imagined providing such an option of
pg_regress or TAP test for the developers.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Masahiko Sawada wrote:
On Fri, Jun 23, 2017 at 3:31 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Fri, Jun 23, 2017 at 3:17 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:Initially, I had naively set wal_consistency_check = all before running
make installcheck and then had to wait for a long time to confirm that WAL
generated by the gin test indeed caused consistency check failure on the
standby with the v1 patch.wal_consistency_check = gin would have saved you a lot of I/O.
But I can see Sawada-san's point that there should be some way for
developers writing code that better had gone through WAL consistency
checking facility to do it without much hassle. But then again, it may
not be that frequent to need that.Yeah, it should be optional. I imagined providing such an option of
pg_regress or TAP test for the developers.
As far as I know it is possible to have third-party modules that extend
the buildfarm client script so that it runs additional tests that the
standard ones. You could have a custom module installed in some
powerful machine of yours that runs the WAL consistency check and report
the results to the buildfarm. A single animal running that test should
be enough, right?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 23, 2017 at 11:17 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
That was it, thanks for the pointer.
GinInitMetabuffer() sets up pd_lower and pd_upper anyway using
PageInit so the check of PageIsVerified is guaranteed to work in any
case. Upgraded pages will still have their pd_lower set to the
previous values, and new pages will have the optimization. So this
patch is actually harmless for past pages, while newer ones are seen
as more compressible.
Attached updated patch, which I confirmed, passes wal_consistency_check = gin.
I have spent some time looking at this patch, playing with pg_upgrade
to check the state of the page upgraded. And this looks good to me.
One thing that I noticed is that this optimization could as well
happen for spgist meta pages. What do others think?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jun 24, 2017 at 1:35 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Masahiko Sawada wrote:
On Fri, Jun 23, 2017 at 3:31 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Fri, Jun 23, 2017 at 3:17 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:Initially, I had naively set wal_consistency_check = all before running
make installcheck and then had to wait for a long time to confirm that WAL
generated by the gin test indeed caused consistency check failure on the
standby with the v1 patch.wal_consistency_check = gin would have saved you a lot of I/O.
But I can see Sawada-san's point that there should be some way for
developers writing code that better had gone through WAL consistency
checking facility to do it without much hassle. But then again, it may
not be that frequent to need that.Yeah, it should be optional. I imagined providing such an option of
pg_regress or TAP test for the developers.As far as I know it is possible to have third-party modules that extend
the buildfarm client script so that it runs additional tests that the
standard ones. You could have a custom module installed in some
powerful machine of yours that runs the WAL consistency check and report
the results to the buildfarm. A single animal running that test should
be enough, right?
Yes, thank you for the information. It's a good idea. I'll try it.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 26, 2017 at 10:54 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Fri, Jun 23, 2017 at 11:17 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:That was it, thanks for the pointer.
GinInitMetabuffer() sets up pd_lower and pd_upper anyway using
PageInit so the check of PageIsVerified is guaranteed to work in any
case. Upgraded pages will still have their pd_lower set to the
previous values, and new pages will have the optimization. So this
patch is actually harmless for past pages, while newer ones are seen
as more compressible.Attached updated patch, which I confirmed, passes wal_consistency_check = gin.
I have spent some time looking at this patch, playing with pg_upgrade
to check the state of the page upgraded. And this looks good to me.
One thing that I noticed is that this optimization could as well
happen for spgist meta pages. What do others think?
Good point. I think it could happen for brin meta page as well.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
