On Sat, Oct 3, 2015 at 12:23 AM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > Masahiko Sawada wrote: >
Thank you for taking time to review this feature.
Attached the latest version patch (v15).
>> @@ -2972,10 +2981,15 @@ l1:
>> */
>> PageSetPrunable(page, xid);
>>
>> + /* clear PD_ALL_VISIBLE and PD_ALL_FORZEN flags */
>
> Typo "FORZEN".
Fixed.
>
>> if (PageIsAllVisible(page))
>> {
>> all_visible_cleared = true;
>> +
>> + /* all-frozen information is also cleared at the same time */
>> PageClearAllVisible(page);
>> + PageClearAllFrozen(page);
>
> I wonder if it makes sense to have a macro to clear both in unison,
> which seems a very common pattern.
>
Fixed.
>
>> diff --git a/src/backend/access/heap/visibilitymap.c
>> b/src/backend/access/heap/visibilitymap.c
>> index 7c38772..a284b85 100644
>> --- a/src/backend/access/heap/visibilitymap.c
>> +++ b/src/backend/access/heap/visibilitymap.c
>> @@ -21,33 +21,45 @@
>> *
>> * NOTES
>> *
>> - * The visibility map is a bitmap with one bit per heap page. A set bit
>> means
>> - * that all tuples on the page are known visible to all transactions, and
>> - * therefore the page doesn't need to be vacuumed. The map is conservative
>> in
>> - * the sense that we make sure that whenever a bit is set, we know the
>> - * condition is true, but if a bit is not set, it might or might not be
>> true.
>> + * The visibility map is a bitmap with two bits (all-visible and all-frozen)
>> + * per heap page. A set all-visible bit means that all tuples on the page
>> are
>> + * known visible to all transactions, and therefore the page doesn't need to
>> + * be vacuumed. A set all-frozen bit means that all tuples on the page are
>> + * completely frozen, and therefore the page doesn't need to be vacuumed
>> even
>> + * if whole table scanning vacuum is required (e.g. anti-wraparound vacuum).
>> + * A all-frozen bit must be set only when the page is already all-visible.
>> + * That is, all-frozen bit is always set with all-visible bit.
>
> "A all-frozen" -> "The all-frozen" (but "A set all-xyz" is correct).
Fixed.
>
>> * When we *set* a visibility map during VACUUM, we must write WAL. This
>> may
>> * seem counterintuitive, since the bit is basically a hint: if it is clear,
>> - * it may still be the case that every tuple on the page is visible to all
>> - * transactions; we just don't know that for certain. The difficulty is
>> that
>> - * there are two bits which are typically set together: the PD_ALL_VISIBLE
>> bit
>> - * on the page itself, and the visibility map bit. If a crash occurs after
>> the
>> - * visibility map page makes it to disk and before the updated heap page
>> makes
>> - * it to disk, redo must set the bit on the heap page. Otherwise, the next
>> - * insert, update, or delete on the heap page will fail to realize that the
>> - * visibility map bit must be cleared, possibly causing index-only scans to
>> - * return wrong answers.
>> + * it may still be the case that every tuple on the page is visible or
>> frozen
>> + * to all transactions; we just don't know that for certain. The
>> difficulty is
>> + * that there are two bits which are typically set together: the
>> PD_ALL_VISIBLE
>> + * or PD_ALL_FROZEN bit on the page itself, and the visibility map bit. If
>> a
>> + * crash occurs after the visibility map page makes it to disk and before
>> the
>> + * updated heap page makes it to disk, redo must set the bit on the heap
>> page.
>> + * Otherwise, the next insert, update, or delete on the heap page will fail
>> to
>> + * realize that the visibility map bit must be cleared, possibly causing
>> index-only
>> + * scans to return wrong answers.
>
> In the "The difficulty ..." para, I would add the word "corresponding" before
> "visibility". Otherwise, it is not clear what the plural means exactly.
Fixed.
>> * VACUUM will normally skip pages for which the visibility map bit is set;
>> * such pages can't contain any dead tuples and therefore don't need
>> vacuuming.
>> - * The visibility map is not used for anti-wraparound vacuums, because
>> + * The visibility map is not used for anti-wraparound vacuums before 9.5,
>> because
>> * an anti-wraparound vacuum needs to freeze tuples and observe the latest
>> xid
>> * present in the table, even on pages that don't have any dead tuples.
>> + * 9.6 or later, the visibility map has a additional bit which indicates
>> all tuple
>> + * on single page has been completely forzen, so the visibility map is also
>> used for
>> + * anti-wraparound vacuums.
>
> This should not mention database versions. Just explain how the code
> behaves today, not how it behaved in the past. Those who want to
> understand how it behaved in 9.5 can read the 9.5 code. (Again typo
> "forzen".)
Changed these comment.
Sorry for the same typo frequently..
>> @@ -1115,6 +1187,14 @@ lazy_scan_heap(Relation onerel, LVRelStats
>> *vacrelstats,
>> tups_vacuumed,
>> vacuumed_pages)));
>>
>> /*
>> + * This information would be effective for how much effect all-frozen
>> bit
>> + * of VM had for freezing tuples.
>> + */
>> + ereport(elevel,
>> + (errmsg("Skipped %d frozen pages acoording to
>> visibility map",
>> + vacrelstats->vmskipped_frozen_pages)));
>
> Message must start on lowercase letter. I don't understand what the
> comment means. Can you rephrase it?
Fixed.
>> @@ -1779,10 +1873,12 @@ vac_cmp_itemptr(const void *left, const void *right)
>> /*
>> * Check if every tuple in the given page is visible to all current and
>> future
>> * transactions. Also return the visibility_cutoff_xid which is the highest
>> - * xmin amongst the visible tuples.
>> + * xmin amongst the visible tuples, and all_forzen which implies that all
>> tuples
>> + * of this page are frozen.
>
> Typo "forzen" here again.
Fixed.
>> @@ -201,6 +239,110 @@ copy_file(const char *srcfile, const char *dstfile,
>> bool force)
>> #endif
>>
>>
>> +/*
>> + * rewriteVisibilitymap()
>> + *
>> + * A additional bit which indicates that all tuples on page is completely
>> + * frozen is added into visibility map at PG 9.6. So the format of
>> visibiilty
>> + * map has been changed.
>> + * Copies a visibility map file while adding all-frozen bit(0) into each
>> bit.
>> + */
>> +static const char *
>> +rewriteVisibilitymap(const char *fromfile, const char *tofile, bool force)
>> +{
>> +#define REWRITE_BUF_SIZE (50 * BLCKSZ)
>> +#define BITS_PER_HEAPBLOCK 2
>> +
>> + int src_fd, dst_fd;
>> + uint16 vm_bits;
>> + ssize_t nbytes;
>> + char *buffer;
>> + int ret = 0;
>> + int save_errno = 0;
>> +
>> + if ((fromfile == NULL) || (tofile == NULL))
>> + {
>> + errno = EINVAL;
>> + return getErrorText(errno);
>> + }
>> +
>> + if ((src_fd = open(fromfile, O_RDONLY, 0)) < 0)
>> + return getErrorText(errno);
>> +
>> + if ((dst_fd = open(tofile, O_RDWR | O_CREAT | (force ? 0 : O_EXCL),
>> S_IRUSR | S_IWUSR)) < 0)
>> + {
>> + save_errno = errno;
>> + if (src_fd != 0)
>> + close(src_fd);
>> +
>> + errno = save_errno;
>> + return getErrorText(errno);
>> + }
>> +
>> + buffer = (char *) pg_malloc(REWRITE_BUF_SIZE);
>> +
>> + /* Copy page header data in advance */
>> + if ((nbytes = read(src_fd, buffer, MAXALIGN(SizeOfPageHeaderData))) <=
>> 0)
>> + {
>> + save_errno = errno;
>> + return getErrorText(errno);
>> + }
>
> Not clear why you bother with save_errno in this path. Forgot to
> close()? (Though I wonder why you bother to close() if the program is
> going to exit shortly thereafter anyway.)
Fixed.
>> diff --git a/src/bin/pg_upgrade/pg_upgrade.h
>> b/src/bin/pg_upgrade/pg_upgrade.h
>> index 13aa891..fc92a5f 100644
>> --- a/src/bin/pg_upgrade/pg_upgrade.h
>> +++ b/src/bin/pg_upgrade/pg_upgrade.h
>> @@ -112,6 +112,11 @@ extern char *output_files[];
>> #define VISIBILITY_MAP_CRASHSAFE_CAT_VER 201107031
>>
>> /*
>> + * The format of visibility map changed with this 9.6 commit,
>> + *
>> + */
>> +#define VISIBILITY_MAP_FROZEN_BIT_CAT_VER 201509181
>
> Useless empty line in comment.
Fixed.
>> diff --git a/src/common/relpath.c b/src/common/relpath.c
>> index 66dfef1..52ff14e 100644
>> --- a/src/common/relpath.c
>> +++ b/src/common/relpath.c
>> @@ -30,6 +30,9 @@
>> * If you add a new entry, remember to update the errhint in
>> * forkname_to_number() below, and update the SGML documentation for
>> * pg_relation_size().
>> + * 9.6 or later, the visibility map fork name is changed from "vm" to
>> + * "vfm" bacause visibility map has not only information about all-visible
>> + * but also information about all-frozen.
>> */
>> const char *const forkNames[] = {
>> "main", /* MAIN_FORKNUM */
>
> Drop the change in comment? There's no "vfm" in this version of the
> patch, is there?
Fixed.
Regards,
--
Masahiko Sawada
000_add_frozen_bit_into_visibilitymap_v15.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
