On 2017/06/19 23:31, Tom Lane wrote:
> Amit Kapila <amit.kapil...@gmail.com> writes:
>> On Mon, Jun 19, 2017 at 11:37 AM, Amit Langote
>> <langote_amit...@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
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index d03d59da6a..b1755c6f1c 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -372,6 +372,13 @@ GinInitMetabuffer(Buffer b)
metadata->nDataPages = 0;
metadata->nEntries = 0;
metadata->ginVersion = GIN_CURRENT_VERSION;
+
+ /*
+ * Set pd_lower just past the end of the metadata. This is not
essential
+ * but it makes the page look compressible to xlog.c.
+ */
+ ((PageHeader) page)->pd_lower =
+ ((char *) metadata + sizeof(GinMetaPageData)) - (char
*) page;
}
/*
diff --git a/src/backend/access/gin/ginxlog.c b/src/backend/access/gin/ginxlog.c
index 7ba04e324f..95b842bef0 100644
--- a/src/backend/access/gin/ginxlog.c
+++ b/src/backend/access/gin/ginxlog.c
@@ -776,18 +776,11 @@ gin_mask(char *pagedata, BlockNumber blkno)
mask_page_hint_bits(page);
/*
- * GIN metapage doesn't use pd_lower/pd_upper. Other page types do.
Hence,
- * we need to apply masking for those pages.
+ * For GIN_DELETED page, the page is initialized to empty. Hence, mask
+ * the page content.
*/
- if (opaque->flags != GIN_META)
- {
- /*
- * For GIN_DELETED page, the page is initialized to empty.
Hence, mask
- * the page content.
- */
- if (opaque->flags & GIN_DELETED)
- mask_page_content(page);
- else
- mask_unused_space(page);
- }
+ if (opaque->flags & GIN_DELETED)
+ mask_page_content(page);
+ else
+ mask_unused_space(page);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
