On Thu, Mar 10, 2016 at 3:27 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Thank you for reviewing!
> Attached updated patch.
>
>
> On Thu, Mar 10, 2016 at 3:37 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Mar 9, 2016 at 9:09 AM, Masahiko Sawada
>> <sawada.mshk@gmail.com> wrote: Attached latest 2 patches.
>>> * 000 patch : Incorporated the review comments and made rewriting
>>> logic more clearly.
>>
>> That's better, thanks. But your comments don't survive pgindent.
>> After running pgindent, I get this:
>>
>> + /*
>> + * These old_* variables point to old visibility map page.
>> + *
>> + * cur_old : Points to current position on old
>> page. blkend_old :
>> + * Points to end of old block. break_old : Points to
>> old page break
>> + * position for rewriting a new page. After wrote a
>> new page, old_end
>> + * proceeds rewriteVmBytesPerPgae bytes.
>> + */
>>
>> You need to either surround this sort of thing with dashes to make
>> pgindent ignore it, or, probably better, rewrite it using complete
>> sentences that together form a paragraph.
>
> Fixed.
>
>>
>> + Oid pg_database_oid; /* OID of
>> pg_database relation */
>>
>> Not used anywhere?
>
> Fixed.
>
>> Instead of vm_need_rewrite, how about vm_must_add_frozenbit?
>
> Fixed.
>
>> Can you explain the changes to test.sh?
>
> Current regression test scenario is,
> 1. Do 'make check' on pre-upgrade cluster
> 2. Dump relallvisible values of all relation in pre-upgrade cluster to
> vm_test1.txt
> 3. Do pg_upgrade
> 4. Do analyze (not vacuum), dump relallvisibile values of all relation
> in post-upgrade cluster to vm_test2.txt
> 5. Compare between vm_test1.txt and vm_test2.txt
>
> That is, regression test compares between relallvisible values in
> pre-upgrade cluster and post-upgrade cluster.
> But because test.sh always uses pre/post clusters with same catalog
> version, I realized that we cannot ensure that visibility map
> rewriting is processed successfully on test.sh framework.
> Rewriting visibility map never be executed.
> We might need to have another framework for rewriting visibility map page..
>
After some further thought, I thought that it's better to add check
logic for result of rewriting visibility map to upgrading logic rather
than regression test in order to ensure that rewriting visibility map
has been successfully done.
As a draft, attached patch checks the result of rewriting visibility
map after rewrote for each relation as a routine of pg_upgrade.
The disadvantage point of this is that we need to scan each visibility
map page for 2 times.
But since visibility map size would not be so large, it would not bad.
Thoughts?
Regards,
--
Regards,
--
Masahiko Sawada