Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-19 Thread Teodor Sigaev

Thank you, both patches are pushed.

Alexander Korotkov wrote:
On Wed, Apr 18, 2018 at 9:21 PM, Peter Geoghegan > wrote:


On Wed, Apr 18, 2018 at 11:12 AM, Fujii Masao > wrote:
> On Thu, Apr 5, 2018 at 1:29 AM, Teodor Sigaev > wrote:
>> Skip full index scan during cleanup of B-tree indexes when possible
>
> This commit added XLOG_BTREE_META_CLEANUP, so btree_desc() and
> btree_identify() should be updated so that they handle 
XLOG_BTREE_META_CLEANUP.
> But ISTM that you forgot doing that.

Thank you for notice.  See attached 
bt-vacuum-cleanup-wal-record-desc-identify.patch fixing that.


Another thing that I noticed is that the metapage stores
btm_last_cleanup_num_heap_tuples as a float4, even though
xl_btree_metadata stores it as a double. I suggest that both places
store it as float8, to be consistent. (It should not be double because
we always avoid using anything other types with explicit typedef'd
widths in WAL records.)


Good catch, thank you!  I also agree that both these fields should be of float8 
type.

Please, find attached bt-vacuum-cleanup-float8-num-heap-tuples.patch fixing 
that.

--
Alexander Korotkov
Postgres Professional:http://www.postgrespro.com 
The Russian Postgres Company



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/



Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-19 Thread Alexander Korotkov
On Thu, Apr 19, 2018 at 9:19 AM, Teodor Sigaev  wrote:

> Another thing that I noticed is that the metapage stores
>> btm_last_cleanup_num_heap_tuples as a float4, even though
>> xl_btree_metadata stores it as a double. I suggest that both places
>> store it as float8, to be consistent. (It should not be double because
>> we always avoid using anything other types with explicit typedef'd
>> widths in WAL records.)
>>
>>
>> Good catch, thank you!  I also agree that both these fields should be of
>> float8 type.
>> Please, find attached bt-vacuum-cleanup-float8-num-heap-tuples.patch
>> fixing that.
>>
>
> Hm. patch changes on-disk representation of btree meta page. And there is
> no support to upgrade meta page since 857f9c36 commit (and should not,
> because that versions wasn't relesead), to prevent possible false positive
> bug reports I suggest to bump catversion. Objections?


I agree, catversion should be bumped for this patch.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-19 Thread Teodor Sigaev

Another thing that I noticed is that the metapage stores
btm_last_cleanup_num_heap_tuples as a float4, even though
xl_btree_metadata stores it as a double. I suggest that both places
store it as float8, to be consistent. (It should not be double because
we always avoid using anything other types with explicit typedef'd
widths in WAL records.)


Good catch, thank you!  I also agree that both these fields should be of 
float8 type.
Please, find attached bt-vacuum-cleanup-float8-num-heap-tuples.patch 
fixing that.


Hm. patch changes on-disk representation of btree meta page. And there 
is no support to upgrade meta page since 857f9c36 commit (and should 
not, because that versions wasn't relesead), to prevent possible false 
positive bug reports I suggest to bump catversion. Objections?



--
Teodor Sigaev  E-mail: teo...@sigaev.ru
  WWW: http://www.sigaev.ru/



Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-18 Thread Alexander Korotkov
On Wed, Apr 18, 2018 at 9:21 PM, Peter Geoghegan  wrote:

> On Wed, Apr 18, 2018 at 11:12 AM, Fujii Masao 
> wrote:
> > On Thu, Apr 5, 2018 at 1:29 AM, Teodor Sigaev  wrote:
> >> Skip full index scan during cleanup of B-tree indexes when possible
> >
> > This commit added XLOG_BTREE_META_CLEANUP, so btree_desc() and
> > btree_identify() should be updated so that they handle
> XLOG_BTREE_META_CLEANUP.
> > But ISTM that you forgot doing that.
>

Thank you for notice.  See attached
bt-vacuum-cleanup-wal-record-desc-identify.patch fixing that.

Another thing that I noticed is that the metapage stores
> btm_last_cleanup_num_heap_tuples as a float4, even though
> xl_btree_metadata stores it as a double. I suggest that both places
> store it as float8, to be consistent. (It should not be double because
> we always avoid using anything other types with explicit typedef'd
> widths in WAL records.)


Good catch, thank you!  I also agree that both these fields should be of
float8 type.
Please, find attached bt-vacuum-cleanup-float8-num-heap-tuples.patch fixing
that.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


bt-vacuum-cleanup-wal-record-desc-identify.patch
Description: Binary data


bt-vacuum-cleanup-float8-num-heap-tuples.patch
Description: Binary data


Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-18 Thread Peter Geoghegan
On Wed, Apr 18, 2018 at 11:12 AM, Fujii Masao  wrote:
> On Thu, Apr 5, 2018 at 1:29 AM, Teodor Sigaev  wrote:
>> Skip full index scan during cleanup of B-tree indexes when possible
>
> This commit added XLOG_BTREE_META_CLEANUP, so btree_desc() and
> btree_identify() should be updated so that they handle 
> XLOG_BTREE_META_CLEANUP.
> But ISTM that you forgot doing that.

Another thing that I noticed is that the metapage stores
btm_last_cleanup_num_heap_tuples as a float4, even though
xl_btree_metadata stores it as a double. I suggest that both places
store it as float8, to be consistent. (It should not be double because
we always avoid using anything other types with explicit typedef'd
widths in WAL records.)

-- 
Peter Geoghegan



Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-18 Thread Fujii Masao
On Thu, Apr 5, 2018 at 1:29 AM, Teodor Sigaev  wrote:
> Skip full index scan during cleanup of B-tree indexes when possible

This commit added XLOG_BTREE_META_CLEANUP, so btree_desc() and
btree_identify() should be updated so that they handle XLOG_BTREE_META_CLEANUP.
But ISTM that you forgot doing that.

Regards,

-- 
Fujii Masao



Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-05 Thread Alexander Korotkov
On Thu, Apr 5, 2018 at 3:24 PM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Thu, Apr 5, 2018 at 2:26 PM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
>
>> On Thu, Apr 5, 2018 at 6:28 AM, Tom Lane  wrote:
>>
>>> Peter Geoghegan  writes:
>>> >>> TRAP: FailedAssertion("!(metad->btm_version == 3)", File:
>>> >>> "/home/pg/postgresql/root/build/../source/src/backend/access
>>> /nbtree/nbtpage.c",
>>> >>> Line: 619)
>>>
>>> >> Hm, buildfarm's not complaining --- what's the test case?
>>>
>>> > This was discovered while testing/reviewing the latest version of the
>>> > INCLUDE covering indexes patch. It now seems to be unrelated.
>>>
>>> Oh, wait ... I wonder if you saw that because you were running a new
>>> backend without having re-initdb'd?  Once you had re-initdb'd, then
>>> of course there would be no old-format btree indexes anywhere.  But
>>> if you hadn't, then anyplace that was not prepared to cope with the
>>> old header format would complain about pre-existing indexes.
>>>
>>> In short, this sounds like a place that did not get the memo about
>>> how to cope with un-upgraded indexes.
>>>
>>
>> That's an issue, because meta-page should be upgraded "on the fly".
>> That was tested, but perhaps without assertions.  I'll investigate more on
>> this an propose a fix.
>>
>
> So, "Assert(metad->btm_version == BTREE_VERSION)" was failed, because
> metadata was copied from meta page to rel->rd_amcache "as is" with
> metad->btm_version == BTREE_MIN_VERSION.
>
> Without assert everything works fine, because extended metadata fields are
> never used from rel->rd_amcache.  So my first intention was to relax this
> assert to Assert(metad->btm_version >= BTREE_MIN_VERSION && metad->btm_version
> <= BTREE_VERSION).
> But then I still have concern that we copy metadata beyond pd_lower
> when metapage is in old format.
>
> memcpy(rel->rd_amcache, metad, sizeof(BTMetaPageData));
>
> This is why I decided to write separate function to handle caching of
> metadata,
> which takes care about filling unavailable fields of metadata with default
> values.
> I also made same fix for pageinspect.  Patch is attached.
>

I forgot to note, that I've tested this patch in following way.  I did
initdb and
created some tables and indexes on eac93e20af.  And then used the
same cluster on patched master including index scans, some
inserts/updates/deletes,
bt_metap(), vacuum and so on.  Everything worked fine.
Sorry, that I didn't test that enough before.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-05 Thread Alexander Korotkov
On Thu, Apr 5, 2018 at 2:26 PM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Thu, Apr 5, 2018 at 6:28 AM, Tom Lane  wrote:
>
>> Peter Geoghegan  writes:
>> >>> TRAP: FailedAssertion("!(metad->btm_version == 3)", File:
>> >>> "/home/pg/postgresql/root/build/../source/src/backend/access
>> /nbtree/nbtpage.c",
>> >>> Line: 619)
>>
>> >> Hm, buildfarm's not complaining --- what's the test case?
>>
>> > This was discovered while testing/reviewing the latest version of the
>> > INCLUDE covering indexes patch. It now seems to be unrelated.
>>
>> Oh, wait ... I wonder if you saw that because you were running a new
>> backend without having re-initdb'd?  Once you had re-initdb'd, then
>> of course there would be no old-format btree indexes anywhere.  But
>> if you hadn't, then anyplace that was not prepared to cope with the
>> old header format would complain about pre-existing indexes.
>>
>> In short, this sounds like a place that did not get the memo about
>> how to cope with un-upgraded indexes.
>>
>
> That's an issue, because meta-page should be upgraded "on the fly".
> That was tested, but perhaps without assertions.  I'll investigate more on
> this an propose a fix.
>

So, "Assert(metad->btm_version == BTREE_VERSION)" was failed, because
metadata was copied from meta page to rel->rd_amcache "as is" with
metad->btm_version == BTREE_MIN_VERSION.

Without assert everything works fine, because extended metadata fields are
never used from rel->rd_amcache.  So my first intention was to relax this
assert to Assert(metad->btm_version >= BTREE_MIN_VERSION && metad->btm_version
<= BTREE_VERSION).
But then I still have concern that we copy metadata beyond pd_lower
when metapage is in old format.

memcpy(rel->rd_amcache, metad, sizeof(BTMetaPageData));

This is why I decided to write separate function to handle caching of
metadata,
which takes care about filling unavailable fields of metadata with default
values.
I also made same fix for pageinspect.  Patch is attached.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


btcleanup-metapage-fix.patch
Description: Binary data


Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-05 Thread Alexander Korotkov
On Thu, Apr 5, 2018 at 6:28 AM, Tom Lane  wrote:

> Peter Geoghegan  writes:
> >>> TRAP: FailedAssertion("!(metad->btm_version == 3)", File:
> >>> "/home/pg/postgresql/root/build/../source/src/backend/
> access/nbtree/nbtpage.c",
> >>> Line: 619)
>
> >> Hm, buildfarm's not complaining --- what's the test case?
>
> > This was discovered while testing/reviewing the latest version of the
> > INCLUDE covering indexes patch. It now seems to be unrelated.
>
> Oh, wait ... I wonder if you saw that because you were running a new
> backend without having re-initdb'd?  Once you had re-initdb'd, then
> of course there would be no old-format btree indexes anywhere.  But
> if you hadn't, then anyplace that was not prepared to cope with the
> old header format would complain about pre-existing indexes.
>
> In short, this sounds like a place that did not get the memo about
> how to cope with un-upgraded indexes.
>

That's an issue, because meta-page should be upgraded "on the fly".
That was tested, but perhaps without assertions.  I'll investigate more on
this an propose a fix.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-05 Thread Alexander Korotkov
On Thu, Apr 5, 2018 at 1:32 AM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Wed, Apr 4, 2018 at 7:29 PM, Teodor Sigaev  wrote:
>
>> Skip full index scan during cleanup of B-tree indexes when possible
>>
>
> Thank you for committing this.
>
> It appears that patch contains some redundant variabled.  See warnings
> produced
> by gcc-7.
>
> nbtpage.c: In function '_bt_update_meta_cleanup_info':
> nbtpage.c:121:15: warning: variable 'metaopaque' set but not used
> [-Wunused-but-set-variable]
>   BTPageOpaque metaopaque;
>^~
> nbtree.c: In function '_bt_vacuum_needs_cleanup':
> nbtree.c:790:15: warning: variable 'metaopaque' set but not used
> [-Wunused-but-set-variable]
>   BTPageOpaque metaopaque;
>^~
>
> Attached patch fixes this.
>

Teodor already committed [1] better patch [2] from Kyotaro Horiguchi.  This
question is closed.

1.
https://www.postgresql.org/message-id/E1f41xX-00029Y-EO%40gemulon.postgresql.org
2. https://www.postgresql.org/message-id/20180405.181730.
125855581.horiguchi.kyotaro%40lab.ntt.co.jp

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-04 Thread Peter Geoghegan
On Wed, Apr 4, 2018 at 8:28 PM, Tom Lane  wrote:
>> This was discovered while testing/reviewing the latest version of the
>> INCLUDE covering indexes patch. It now seems to be unrelated.
>
> Oh, wait ... I wonder if you saw that because you were running a new
> backend without having re-initdb'd?

Yes. That's what happened.

> Once you had re-initdb'd, then
> of course there would be no old-format btree indexes anywhere.  But
> if you hadn't, then anyplace that was not prepared to cope with the
> old header format would complain about pre-existing indexes.
>
> In short, this sounds like a place that did not get the memo about
> how to cope with un-upgraded indexes.

Sounds plausible.

-- 
Peter Geoghegan



Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-04 Thread Tom Lane
Peter Geoghegan  writes:
>>> TRAP: FailedAssertion("!(metad->btm_version == 3)", File:
>>> "/home/pg/postgresql/root/build/../source/src/backend/access/nbtree/nbtpage.c",
>>> Line: 619)

>> Hm, buildfarm's not complaining --- what's the test case?

> This was discovered while testing/reviewing the latest version of the
> INCLUDE covering indexes patch. It now seems to be unrelated.

Oh, wait ... I wonder if you saw that because you were running a new
backend without having re-initdb'd?  Once you had re-initdb'd, then
of course there would be no old-format btree indexes anywhere.  But
if you hadn't, then anyplace that was not prepared to cope with the
old header format would complain about pre-existing indexes.

In short, this sounds like a place that did not get the memo about
how to cope with un-upgraded indexes.

regards, tom lane



Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-04 Thread Peter Geoghegan
On Wed, Apr 4, 2018 at 5:58 PM, Tom Lane  wrote:
>> TRAP: FailedAssertion("!(metad->btm_version == 3)", File:
>> "/home/pg/postgresql/root/build/../source/src/backend/access/nbtree/nbtpage.c",
>> Line: 619)
>
> Hm, buildfarm's not complaining --- what's the test case?

This was discovered while testing/reviewing the latest version of the
INCLUDE covering indexes patch. It now seems to be unrelated.

Sorry for the noise.

-- 
Peter Geoghegan



Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-04 Thread Michael Paquier
On Wed, Apr 04, 2018 at 08:58:14PM -0400, Tom Lane wrote:
> Peter Geoghegan  writes:
> > I also see an assertion failure within _bt_getrootheight():
> 
> > TRAP: FailedAssertion("!(metad->btm_version == 3)", File:
> > "/home/pg/postgresql/root/build/../source/src/backend/access/nbtree/nbtpage.c",
> > Line: 619)
> 
> Hm, buildfarm's not complaining --- what's the test case?

Hm.  No problems here either with a56e267 and gcc 7.3.  The warnings are
here for sure, and any compiler would complain about those.
--
Michael


signature.asc
Description: PGP signature


Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-04 Thread Tom Lane
Peter Geoghegan  writes:
> I also see an assertion failure within _bt_getrootheight():

> TRAP: FailedAssertion("!(metad->btm_version == 3)", File:
> "/home/pg/postgresql/root/build/../source/src/backend/access/nbtree/nbtpage.c",
> Line: 619)

Hm, buildfarm's not complaining --- what's the test case?

regards, tom lane



Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-04 Thread Peter Geoghegan
On Wed, Apr 4, 2018 at 3:32 PM, Alexander Korotkov
 wrote:
> Hi!
>
> On Wed, Apr 4, 2018 at 7:29 PM, Teodor Sigaev  wrote:
>>
>> Skip full index scan during cleanup of B-tree indexes when possible
>
>
> Thank you for committing this.
>
> It appears that patch contains some redundant variabled.  See warnings
> produced
> by gcc-7.

I also see an assertion failure within _bt_getrootheight():

TRAP: FailedAssertion("!(metad->btm_version == 3)", File:
"/home/pg/postgresql/root/build/../source/src/backend/access/nbtree/nbtpage.c",
Line: 619)

-- 
Peter Geoghegan



Re: pgsql: Skip full index scan during cleanup of B-tree indexes when possi

2018-04-04 Thread Alexander Korotkov
Hi!

On Wed, Apr 4, 2018 at 7:29 PM, Teodor Sigaev  wrote:

> Skip full index scan during cleanup of B-tree indexes when possible
>

Thank you for committing this.

It appears that patch contains some redundant variabled.  See warnings
produced
by gcc-7.

nbtpage.c: In function '_bt_update_meta_cleanup_info':
nbtpage.c:121:15: warning: variable 'metaopaque' set but not used
[-Wunused-but-set-variable]
  BTPageOpaque metaopaque;
   ^~
nbtree.c: In function '_bt_vacuum_needs_cleanup':
nbtree.c:790:15: warning: variable 'metaopaque' set but not used
[-Wunused-but-set-variable]
  BTPageOpaque metaopaque;
   ^~

Attached patch fixes this.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


bt-vacuum-cleanup-fix-warnings.patch
Description: Binary data