pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Add deduplication to nbtree.
22+ messages / 5 participants
[nested] [flat]
* pgsql: Add deduplication to nbtree.
@ 2020-02-26 21:06 Peter Geoghegan <pg@bowt.ie>
2020-02-27 17:25 ` Re: pgsql: Add deduplication to nbtree. Laurenz Albe <laurenz.albe@cybertec.at>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-04-30 18:33 ` Re: pgsql: Add deduplication to nbtree. Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 3 replies; 22+ messages in thread
From: Peter Geoghegan @ 2020-02-26 21:06 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Add deduplication to nbtree.
Deduplication reduces the storage overhead of duplicates in indexes that
use the standard nbtree index access method. The deduplication process
is applied lazily, after the point where opportunistic deletion of
LP_DEAD-marked index tuples occurs. Deduplication is only applied at
the point where a leaf page split would otherwise be required. New
posting list tuples are formed by merging together existing duplicate
tuples. The physical representation of the items on an nbtree leaf page
is made more space efficient by deduplication, but the logical contents
of the page are not changed. Even unique indexes make use of
deduplication as a way of controlling bloat from duplicates whose TIDs
point to different versions of the same logical table row.
The lazy approach taken by nbtree has significant advantages over a GIN
style eager approach. Most individual inserts of index tuples have
exactly the same overhead as before. The extra overhead of
deduplication is amortized across insertions, just like the overhead of
page splits. The key space of indexes works in the same way as it has
since commit dd299df8 (the commit that made heap TID a tiebreaker
column).
Testing has shown that nbtree deduplication can generally make indexes
with about 10 or 15 tuples for each distinct key value about 2.5X - 4X
smaller, even with single column integer indexes (e.g., an index on a
referencing column that accompanies a foreign key). The final size of
single column nbtree indexes comes close to the final size of a similar
contrib/btree_gin index, at least in cases where GIN's posting list
compression isn't very effective. This can significantly improve
transaction throughput, and significantly reduce the cost of vacuuming
indexes.
A new index storage parameter (deduplicate_items) controls the use of
deduplication. The default setting is 'on', so all new B-Tree indexes
automatically use deduplication where possible. This decision will be
reviewed at the end of the Postgres 13 beta period.
There is a regression of approximately 2% of transaction throughput with
synthetic workloads that consist of append-only inserts into a table
with several non-unique indexes, where all indexes have few or no
repeated values. The underlying issue is that cycles are wasted on
unsuccessful attempts at deduplicating items in non-unique indexes.
There doesn't seem to be a way around it short of disabling
deduplication entirely. Note that deduplication of items in unique
indexes is fairly well targeted in general, which avoids the problem
there (we can use a special heuristic to trigger deduplication passes in
unique indexes, since we're specifically targeting "version bloat").
Bump XLOG_PAGE_MAGIC because xl_btree_vacuum changed.
No bump in BTREE_VERSION, since the representation of posting list
tuples works in a way that's backwards compatible with version 4 indexes
(i.e. indexes built on PostgreSQL 12). However, users must still
REINDEX a pg_upgrade'd index to use deduplication, regardless of the
Postgres version they've upgraded from. This is the only way to set the
new nbtree metapage flag indicating that deduplication is generally
safe.
Author: Anastasia Lubennikova, Peter Geoghegan
Reviewed-By: Peter Geoghegan, Heikki Linnakangas
Discussion:
https://postgr.es/m/55E4051B.7020209@postgrespro.ru
https://postgr.es/m/4ab6e2db-bcee-f4cf-0916-3a06e6ccbb55@postgrespro.ru
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/0d861bbb702f8aa05c2a4e3f1650e7e8df8c8c27
Modified Files
--------------
contrib/amcheck/verify_nbtree.c | 231 +++++++--
doc/src/sgml/btree.sgml | 201 +++++++-
doc/src/sgml/charset.sgml | 9 +-
doc/src/sgml/citext.sgml | 7 +-
doc/src/sgml/func.sgml | 9 +-
doc/src/sgml/ref/create_index.sgml | 44 +-
src/backend/access/common/reloptions.c | 10 +
src/backend/access/index/genam.c | 4 +
src/backend/access/nbtree/Makefile | 1 +
src/backend/access/nbtree/README | 133 ++++-
src/backend/access/nbtree/nbtdedup.c | 830 ++++++++++++++++++++++++++++++
src/backend/access/nbtree/nbtinsert.c | 388 ++++++++++++--
src/backend/access/nbtree/nbtpage.c | 246 ++++++++-
src/backend/access/nbtree/nbtree.c | 171 +++++-
src/backend/access/nbtree/nbtsearch.c | 272 +++++++++-
src/backend/access/nbtree/nbtsort.c | 193 ++++++-
src/backend/access/nbtree/nbtsplitloc.c | 39 +-
src/backend/access/nbtree/nbtutils.c | 196 +++++--
src/backend/access/nbtree/nbtxlog.c | 268 +++++++++-
src/backend/access/rmgrdesc/nbtdesc.c | 22 +-
src/backend/storage/page/bufpage.c | 11 +-
src/bin/psql/tab-complete.c | 4 +-
src/include/access/nbtree.h | 433 +++++++++++++---
src/include/access/nbtxlog.h | 117 ++++-
src/include/access/rmgrlist.h | 2 +-
src/include/access/xlog_internal.h | 2 +-
src/test/regress/expected/btree_index.out | 20 +-
src/test/regress/sql/btree_index.sql | 22 +-
28 files changed, 3553 insertions(+), 332 deletions(-)
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-02-27 17:25 ` Laurenz Albe <laurenz.albe@cybertec.at>
2020-02-27 17:26 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2 siblings, 1 reply; 22+ messages in thread
From: Laurenz Albe @ 2020-02-27 17:25 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; pgsql-committers@lists.postgresql.org
On Wed, 2020-02-26 at 21:06 +0000, Peter Geoghegan wrote:
> Add deduplication to nbtree.
This is great! Thanks!
Are no changes to the "pageinspect" contrib required?
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-02-27 17:25 ` Re: pgsql: Add deduplication to nbtree. Laurenz Albe <laurenz.albe@cybertec.at>
@ 2020-02-27 17:26 ` Peter Geoghegan <pg@bowt.ie>
2020-02-29 02:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 22+ messages in thread
From: Peter Geoghegan @ 2020-02-27 17:26 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Thu, Feb 27, 2020 at 9:25 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> This is great! Thanks!
Thanks!
> Are no changes to the "pageinspect" contrib required?
There are. Those will be pushed either today or tomorrow.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-02-27 17:25 ` Re: pgsql: Add deduplication to nbtree. Laurenz Albe <laurenz.albe@cybertec.at>
2020-02-27 17:26 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-02-29 02:37 ` Peter Geoghegan <pg@bowt.ie>
0 siblings, 0 replies; 22+ messages in thread
From: Peter Geoghegan @ 2020-02-29 02:37 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Thu, Feb 27, 2020 at 9:26 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > Are no changes to the "pageinspect" contrib required?
>
> There are. Those will be pushed either today or tomorrow.
Attached is a draft patch for this, with updated documentation. I'd
like to push this tomorrow (Saturday), but if you could take a look
over it first, that would be great.
Thanks
--
Peter Geoghegan
Attachments:
[application/octet-stream] v1-0001-Teach-pageinspect-about-nbtree-deduplication.patch (21.9K, ../../CAH2-WzkWacKv2Y5F6VjZccXt68pnfdmkeA3PZc8zh+8KRfUdhA@mail.gmail.com/2-v1-0001-Teach-pageinspect-about-nbtree-deduplication.patch)
download | inline diff:
From b8a1e026e69bf4a641e8be3cf5b15ab7d532fa61 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Sun, 16 Feb 2020 01:16:02 -0800
Subject: [PATCH v1] Teach pageinspect about nbtree deduplication.
Update the bt_metap() function: add a new column that displays the
nbtree metapage's allequalimage field. This field indicates whether or
not deduplication can be assumed safe by nbtree.
Also, add three new columns to contrib/pageinspect's bt_page_items()
function:
* Add a boolean column ("dead") that displays the LP_DEAD bit value for
each non-pivot tuple.
* Add a TID column ("htid") that displays a single heap TID value for
each tuple. This is the TID that is returned by BTreeTupleGetHeapTID(),
so comparable values are shown for pivot tuples, plain non-pivot tuples,
and posting list tuples.
* Add a TID array column ("tids") that displays TIDs from each tuple's
posting list, if any. This works just like the "tids" column from
pageinspect's gin_leafpage_items() function.
This patch is the final patch in the deduplication patch series. The
"dead" column isn't truly related to posting list tuples, but it was
very useful for testing the deduplication patch. The column was used to
make sure that posting list tuples have their LP_DEAD bits set correctly
and consistently.
No version bump for the pageinspect extension, since there hasn't been a
stable Postgres release since the last version bump (the last bump was
part of commit 58b4cb30).
Author: Peter Geoghegan
Discussion: https://postgr.es/m/CAH2-WzmSMmU2eNvY9+a4MNP+z02h6sa-uxZvN3un6jY02ZVBSw@mail.gmail.com
---
contrib/pageinspect/btreefuncs.c | 143 +++++++++++++++---
contrib/pageinspect/expected/btree.out | 7 +
contrib/pageinspect/pageinspect--1.7--1.8.sql | 53 +++++++
doc/src/sgml/pageinspect.sgml | 106 ++++++++-----
4 files changed, 256 insertions(+), 53 deletions(-)
diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index 564c818558..39e36bfce4 100644
--- a/contrib/pageinspect/btreefuncs.c
+++ b/contrib/pageinspect/btreefuncs.c
@@ -31,9 +31,11 @@
#include "access/relation.h"
#include "catalog/namespace.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_type.h"
#include "funcapi.h"
#include "miscadmin.h"
#include "pageinspect.h"
+#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/rel.h"
#include "utils/varlena.h"
@@ -45,6 +47,8 @@ PG_FUNCTION_INFO_V1(bt_page_stats);
#define IS_INDEX(r) ((r)->rd_rel->relkind == RELKIND_INDEX)
#define IS_BTREE(r) ((r)->rd_rel->relam == BTREE_AM_OID)
+#define DatumGetItemPointer(X) ((ItemPointer) DatumGetPointer(X))
+#define ItemPointerGetDatum(X) PointerGetDatum(X)
/* note: BlockNumber is unsigned, hence can't be negative */
#define CHECK_RELATION_BLOCK_RANGE(rel, blkno) { \
@@ -243,6 +247,9 @@ struct user_args
{
Page page;
OffsetNumber offset;
+ bool leafpage;
+ bool rightmost;
+ TupleDesc tupd;
};
/*-------------------------------------------------------
@@ -252,17 +259,25 @@ struct user_args
* ------------------------------------------------------
*/
static Datum
-bt_page_print_tuples(FuncCallContext *fctx, Page page, OffsetNumber offset)
+bt_page_print_tuples(FuncCallContext *fctx, struct user_args *uargs)
{
- char *values[6];
+ Page page = uargs->page;
+ OffsetNumber offset = uargs->offset;
+ bool leafpage = uargs->leafpage;
+ bool rightmost = uargs->rightmost;
+ bool ispivottuple;
+ Datum values[9];
+ bool nulls[9];
HeapTuple tuple;
ItemId id;
IndexTuple itup;
int j;
int off;
int dlen;
- char *dump;
+ char *dump,
+ *datacstring;
char *ptr;
+ ItemPointer htid;
id = PageGetItemId(page, offset);
@@ -272,18 +287,46 @@ bt_page_print_tuples(FuncCallContext *fctx, Page page, OffsetNumber offset)
itup = (IndexTuple) PageGetItem(page, id);
j = 0;
- values[j++] = psprintf("%d", offset);
- values[j++] = psprintf("(%u,%u)",
- ItemPointerGetBlockNumberNoCheck(&itup->t_tid),
- ItemPointerGetOffsetNumberNoCheck(&itup->t_tid));
- values[j++] = psprintf("%d", (int) IndexTupleSize(itup));
- values[j++] = psprintf("%c", IndexTupleHasNulls(itup) ? 't' : 'f');
- values[j++] = psprintf("%c", IndexTupleHasVarwidths(itup) ? 't' : 'f');
+ memset(nulls, 0, sizeof(nulls));
+ values[j++] = DatumGetInt16(offset);
+ values[j++] = ItemPointerGetDatum(&itup->t_tid);
+ values[j++] = Int32GetDatum((int) IndexTupleSize(itup));
+ values[j++] = BoolGetDatum(IndexTupleHasNulls(itup));
+ values[j++] = BoolGetDatum(IndexTupleHasVarwidths(itup));
ptr = (char *) itup + IndexInfoFindDataOffset(itup->t_info);
dlen = IndexTupleSize(itup) - IndexInfoFindDataOffset(itup->t_info);
+
+ /*
+ * Make sure that "data" column does not include posting list or pivot
+ * tuple representation of heap TID(s).
+ *
+ * Note: BTreeTupleIsPivot() won't work reliably on !heapkeyspace indexes
+ * (those built before BTREE_VERSION 4), but we have no way of determining
+ * if this page came from a !heapkeyspace index. We may only have a bytea
+ * nbtree page image to go on, so there is no metapage that we can check
+ * reliably.
+ *
+ * That's okay here because BTreeTupleIsPivot() can only return false for
+ * a !heapkeyspace pivot, never true for a !heapkeyspace non-pivot. Since
+ * heap TID isn't part of the keyspace in a !heapkeyspace index anyway,
+ * there cannot possibly be a pivot tuple heap TID representation that we
+ * fail to make an adjustment for. A !heapkeyspace index can have
+ * BTreeTupleIsPivot() return true (due to things like suffix truncation
+ * for INCLUDE indexes in Postgres v11), but when that happens
+ * BTreeTupleGetHeapTID() can be trusted to work reliably (i.e. return
+ * NULL).
+ *
+ * Note: BTreeTupleIsPosting() works reliably, even with !heapkeyspace
+ * indexes.
+ */
+ if (BTreeTupleIsPosting(itup))
+ dlen -= IndexTupleSize(itup) - BTreeTupleGetPostingOffset(itup);
+ else if (BTreeTupleIsPivot(itup) && BTreeTupleGetHeapTID(itup) != NULL)
+ dlen -= MAXALIGN(sizeof(ItemPointerData));
+
dump = palloc0(dlen * 3 + 1);
- values[j] = dump;
+ datacstring = dump;
for (off = 0; off < dlen; off++)
{
if (off > 0)
@@ -291,8 +334,62 @@ bt_page_print_tuples(FuncCallContext *fctx, Page page, OffsetNumber offset)
sprintf(dump, "%02x", *(ptr + off) & 0xff);
dump += 2;
}
+ values[j++] = CStringGetTextDatum(datacstring);
+ pfree(datacstring);
- tuple = BuildTupleFromCStrings(fctx->attinmeta, values);
+ /*
+ * We need to work around the BTreeTupleIsPivot() !heapkeyspace limitation
+ * again. Deduce whether or not tuple must be a pivot tuple based on
+ * whether or not the page is a leaf page, as well as the page offset
+ * number of the tuple.
+ */
+ ispivottuple = (!leafpage || (!rightmost && offset == P_HIKEY));
+
+ /* LP_DEAD bit can never be set for pivot tuples, so show a NULL there */
+ if (!ispivottuple)
+ values[j++] = BoolGetDatum(ItemIdIsDead(id));
+ else
+ {
+ Assert(!ItemIdIsDead(id));
+ nulls[j++] = true;
+ }
+
+ htid = BTreeTupleGetHeapTID(itup);
+ if (ispivottuple && !BTreeTupleIsPivot(itup))
+ {
+ /* Don't show bogus heap TID in !heapkeyspace pivot tuple */
+ htid = NULL;
+ }
+
+ if (htid)
+ values[j++] = ItemPointerGetDatum(htid);
+ else
+ nulls[j++] = true;
+
+ if (BTreeTupleIsPosting(itup))
+ {
+ /* Build an array of item pointers */
+ ItemPointer tids;
+ Datum *tids_datum;
+ int nposting;
+
+ tids = BTreeTupleGetPosting(itup);
+ nposting = BTreeTupleGetNPosting(itup);
+ tids_datum = (Datum *) palloc(nposting * sizeof(Datum));
+ for (int i = 0; i < nposting; i++)
+ tids_datum[i] = ItemPointerGetDatum(&tids[i]);
+ values[j++] = PointerGetDatum(construct_array(tids_datum,
+ nposting,
+ TIDOID,
+ sizeof(ItemPointerData),
+ false, 's'));
+ pfree(tids_datum);
+ }
+ else
+ nulls[j++] = true;
+
+ /* Build and return the result tuple */
+ tuple = heap_form_tuple(uargs->tupd, values, nulls);
return HeapTupleGetDatum(tuple);
}
@@ -378,12 +475,15 @@ bt_page_items(PG_FUNCTION_ARGS)
elog(NOTICE, "page is deleted");
fctx->max_calls = PageGetMaxOffsetNumber(uargs->page);
+ uargs->leafpage = P_ISLEAF(opaque);
+ uargs->rightmost = P_RIGHTMOST(opaque);
/* Build a tuple descriptor for our result type */
if (get_call_result_type(fcinfo, NULL, &tupleDesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
+ tupleDesc = BlessTupleDesc(tupleDesc);
- fctx->attinmeta = TupleDescGetAttInMetadata(tupleDesc);
+ uargs->tupd = tupleDesc;
fctx->user_fctx = uargs;
@@ -395,7 +495,7 @@ bt_page_items(PG_FUNCTION_ARGS)
if (fctx->call_cntr < fctx->max_calls)
{
- result = bt_page_print_tuples(fctx, uargs->page, uargs->offset);
+ result = bt_page_print_tuples(fctx, uargs);
uargs->offset++;
SRF_RETURN_NEXT(fctx, result);
}
@@ -463,12 +563,15 @@ bt_page_items_bytea(PG_FUNCTION_ARGS)
elog(NOTICE, "page is deleted");
fctx->max_calls = PageGetMaxOffsetNumber(uargs->page);
+ uargs->leafpage = P_ISLEAF(opaque);
+ uargs->rightmost = P_RIGHTMOST(opaque);
/* Build a tuple descriptor for our result type */
if (get_call_result_type(fcinfo, NULL, &tupleDesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
+ tupleDesc = BlessTupleDesc(tupleDesc);
- fctx->attinmeta = TupleDescGetAttInMetadata(tupleDesc);
+ uargs->tupd = tupleDesc;
fctx->user_fctx = uargs;
@@ -480,7 +583,7 @@ bt_page_items_bytea(PG_FUNCTION_ARGS)
if (fctx->call_cntr < fctx->max_calls)
{
- result = bt_page_print_tuples(fctx, uargs->page, uargs->offset);
+ result = bt_page_print_tuples(fctx, uargs);
uargs->offset++;
SRF_RETURN_NEXT(fctx, result);
}
@@ -510,7 +613,7 @@ bt_metap(PG_FUNCTION_ARGS)
BTMetaPageData *metad;
TupleDesc tupleDesc;
int j;
- char *values[8];
+ char *values[9];
Buffer buffer;
Page page;
HeapTuple tuple;
@@ -557,17 +660,21 @@ bt_metap(PG_FUNCTION_ARGS)
/*
* Get values of extended metadata if available, use default values
- * otherwise.
+ * otherwise. Note that we rely on the assumption that btm_allequalimage
+ * is initialized to zero with indexes that were built on versions prior
+ * to Postgres 13 (just like _bt_metaversion()).
*/
if (metad->btm_version >= BTREE_NOVAC_VERSION)
{
values[j++] = psprintf("%u", metad->btm_oldest_btpo_xact);
values[j++] = psprintf("%f", metad->btm_last_cleanup_num_heap_tuples);
+ values[j++] = metad->btm_allequalimage ? "t" : "f";
}
else
{
values[j++] = "0";
values[j++] = "-1";
+ values[j++] = "f";
}
tuple = BuildTupleFromCStrings(TupleDescGetAttInMetadata(tupleDesc),
diff --git a/contrib/pageinspect/expected/btree.out b/contrib/pageinspect/expected/btree.out
index 07c2dcd771..17bf0c5470 100644
--- a/contrib/pageinspect/expected/btree.out
+++ b/contrib/pageinspect/expected/btree.out
@@ -12,6 +12,7 @@ fastroot | 1
fastlevel | 0
oldest_xact | 0
last_cleanup_num_tuples | -1
+allequalimage | t
SELECT * FROM bt_page_stats('test1_a_idx', 0);
ERROR: block 0 is a meta page
@@ -41,6 +42,9 @@ itemlen | 16
nulls | f
vars | f
data | 01 00 00 00 00 00 00 01
+dead | f
+htid | (0,1)
+tids |
SELECT * FROM bt_page_items('test1_a_idx', 2);
ERROR: block number out of range
@@ -54,6 +58,9 @@ itemlen | 16
nulls | f
vars | f
data | 01 00 00 00 00 00 00 01
+dead | f
+htid | (0,1)
+tids |
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 2));
ERROR: block number 2 is out of range for relation "test1_a_idx"
diff --git a/contrib/pageinspect/pageinspect--1.7--1.8.sql b/contrib/pageinspect/pageinspect--1.7--1.8.sql
index 2a7c4b3516..e34c214c93 100644
--- a/contrib/pageinspect/pageinspect--1.7--1.8.sql
+++ b/contrib/pageinspect/pageinspect--1.7--1.8.sql
@@ -14,3 +14,56 @@ CREATE FUNCTION heap_tuple_infomask_flags(
RETURNS record
AS 'MODULE_PATHNAME', 'heap_tuple_infomask_flags'
LANGUAGE C STRICT PARALLEL SAFE;
+
+--
+-- bt_metap()
+--
+DROP FUNCTION bt_metap(text);
+CREATE FUNCTION bt_metap(IN relname text,
+ OUT magic int4,
+ OUT version int4,
+ OUT root int4,
+ OUT level int4,
+ OUT fastroot int4,
+ OUT fastlevel int4,
+ OUT oldest_xact int4,
+ OUT last_cleanup_num_tuples real,
+ OUT allequalimage boolean)
+AS 'MODULE_PATHNAME', 'bt_metap'
+LANGUAGE C STRICT PARALLEL SAFE;
+
+--
+-- bt_page_items(text, int4)
+--
+DROP FUNCTION bt_page_items(text, int4);
+CREATE FUNCTION bt_page_items(IN relname text, IN blkno int4,
+ OUT itemoffset smallint,
+ OUT ctid tid,
+ OUT itemlen smallint,
+ OUT nulls bool,
+ OUT vars bool,
+ OUT data text,
+ OUT dead boolean,
+ OUT htid tid,
+ OUT tids tid[])
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'bt_page_items'
+LANGUAGE C STRICT PARALLEL SAFE;
+
+--
+-- bt_page_items(bytea)
+--
+DROP FUNCTION bt_page_items(bytea);
+CREATE FUNCTION bt_page_items(IN page bytea,
+ OUT itemoffset smallint,
+ OUT ctid tid,
+ OUT itemlen smallint,
+ OUT nulls bool,
+ OUT vars bool,
+ OUT data text,
+ OUT dead boolean,
+ OUT htid tid,
+ OUT tids tid[])
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'bt_page_items_bytea'
+LANGUAGE C STRICT PARALLEL SAFE;
diff --git a/doc/src/sgml/pageinspect.sgml b/doc/src/sgml/pageinspect.sgml
index 7e2e1487d7..713603a355 100644
--- a/doc/src/sgml/pageinspect.sgml
+++ b/doc/src/sgml/pageinspect.sgml
@@ -300,13 +300,14 @@ test=# SELECT t_ctid, raw_flags, combined_flags
test=# SELECT * FROM bt_metap('pg_cast_oid_index');
-[ RECORD 1 ]-----------+-------
magic | 340322
-version | 3
+version | 4
root | 1
level | 0
fastroot | 1
fastlevel | 0
oldest_xact | 582
last_cleanup_num_tuples | 1000
+allequalimage | f
</screen>
</para>
</listitem>
@@ -329,11 +330,11 @@ test=# SELECT * FROM bt_page_stats('pg_cast_oid_index', 1);
-[ RECORD 1 ]-+-----
blkno | 1
type | l
-live_items | 256
+live_items | 224
dead_items | 0
-avg_item_size | 12
+avg_item_size | 16
page_size | 8192
-free_size | 4056
+free_size | 3668
btpo_prev | 0
btpo_next | 0
btpo | 0
@@ -356,33 +357,67 @@ btpo_flags | 3
<function>bt_page_items</function> returns detailed information about
all of the items on a B-tree index page. For example:
<screen>
-test=# SELECT * FROM bt_page_items('pg_cast_oid_index', 1);
- itemoffset | ctid | itemlen | nulls | vars | data
-------------+---------+---------+-------+------+-------------
- 1 | (0,1) | 12 | f | f | 23 27 00 00
- 2 | (0,2) | 12 | f | f | 24 27 00 00
- 3 | (0,3) | 12 | f | f | 25 27 00 00
- 4 | (0,4) | 12 | f | f | 26 27 00 00
- 5 | (0,5) | 12 | f | f | 27 27 00 00
- 6 | (0,6) | 12 | f | f | 28 27 00 00
- 7 | (0,7) | 12 | f | f | 29 27 00 00
- 8 | (0,8) | 12 | f | f | 2a 27 00 00
+ test=# SELECT itemoffset, ctid, itemlen, nulls, vars, data, dead, htid, tids[0:2] AS some_tids
+ FROM bt_page_items(get_raw_page('tenk2_hundred', 5));
+ itemoffset | ctid | itemlen | nulls | vars | data | dead | htid | some_tids
+------------+-----------+---------+-------+------+-------------------------+------+--------+---------------------
+ 1 | (16,1) | 16 | f | f | 30 00 00 00 00 00 00 00 | | |
+ 2 | (16,8292) | 616 | f | f | 24 00 00 00 00 00 00 00 | f | (1,6) | {"(1,6)","(10,22)"}
+ 3 | (16,8292) | 616 | f | f | 25 00 00 00 00 00 00 00 | f | (1,18) | {"(1,18)","(4,22)"}
+ 4 | (16,8292) | 616 | f | f | 26 00 00 00 00 00 00 00 | f | (4,18) | {"(4,18)","(6,17)"}
+ 5 | (16,8292) | 616 | f | f | 27 00 00 00 00 00 00 00 | f | (1,2) | {"(1,2)","(1,19)"}
+ 6 | (16,8292) | 616 | f | f | 28 00 00 00 00 00 00 00 | f | (2,24) | {"(2,24)","(4,11)"}
+ 7 | (16,8292) | 616 | f | f | 29 00 00 00 00 00 00 00 | f | (2,17) | {"(2,17)","(11,2)"}
+ 8 | (16,8292) | 616 | f | f | 2a 00 00 00 00 00 00 00 | f | (0,25) | {"(0,25)","(3,20)"}
</screen>
- In a B-tree leaf page, <structfield>ctid</structfield> points to a heap tuple.
- In an internal page, the block number part of <structfield>ctid</structfield>
- points to another page in the index itself, while the offset part
- (the second number) is ignored and is usually 1.
+ This is a B-tree leaf page where all tuples that point to the
+ table happen to be posting list tuples (all of which store a
+ total of 100 6 byte TIDs). There is also a single <quote>high
+ key</quote> tuple at <literal>itemoffset</literal> number 1.
+ <structfield>ctid</structfield> is used to encode metadata about
+ each tuple in this example, though leaf page tuples often
+ directly store a heap TID in the <structfield>ctid</structfield>
+ field instead. <structfield>tids</structfield> is the list of
+ TIDs stored as a posting list (this is always NULL for tuples
+ that lack a posting list).
+ </para>
+ <para>
+ In an internal page (not shown), the block number part of
+ <structfield>ctid</structfield> is a <quote>downlink</quote>,
+ which is a block number that points to another page in the index
+ itself. The offset part (the second number) of
+ <structfield>ctid</structfield> is used to encode metadata about
+ the tuple, such as the number of columns present (suffix
+ truncation may have removed unneeded suffix columns).
+ </para>
+ <para>
+ <structfield>htid</structfield> shows a heap TID for the tuple,
+ if any, without regard for the underlying tuple representation.
+ This value may match <structfield>ctid</structfield>, or may be
+ decoded from the alternative representations used by posting
+ list tuples and tuples from internal pages (though note that
+ tuples in internal pages usually have the implementation level
+ heap TID column truncated away, in which case NULL is shown).
</para>
<para>
Note that the first item on any non-rightmost page (any page with
a non-zero value in the <structfield>btpo_next</structfield> field) is the
page's <quote>high key</quote>, meaning its <structfield>data</structfield>
serves as an upper bound on all items appearing on the page, while
- its <structfield>ctid</structfield> field is meaningless. Also, on non-leaf
- pages, the first real data item (the first item that is not a high
- key) is a <quote>minus infinity</quote> item, with no actual value
- in its <structfield>data</structfield> field. Such an item does have a valid
- downlink in its <structfield>ctid</structfield> field, however.
+ its <structfield>ctid</structfield> field does not point to
+ another block (high keys use the same representation as tuples
+ on internal pages). Also, on internal pages, the first real
+ data item (the first item that is not a high key) is a
+ <quote>minus infinity</quote> item, with no actual value in its
+ <structfield>data</structfield> field. Such an item does have a
+ valid downlink in its <structfield>ctid</structfield> field,
+ however.
+ </para>
+ <para>
+ See <xref linkend="btree-structure"/> for an explanation of
+ terms like leaf page and internal page. See <xref
+ linkend="btree-deduplication"/> for an explanation of
+ deduplication and posting lists.
</para>
</listitem>
</varlistentry>
@@ -402,17 +437,18 @@ test=# SELECT * FROM bt_page_items('pg_cast_oid_index', 1);
with <function>get_raw_page</function> should be passed as argument. So
the last example could also be rewritten like this:
<screen>
-test=# SELECT * FROM bt_page_items(get_raw_page('pg_cast_oid_index', 1));
- itemoffset | ctid | itemlen | nulls | vars | data
-------------+---------+---------+-------+------+-------------
- 1 | (0,1) | 12 | f | f | 23 27 00 00
- 2 | (0,2) | 12 | f | f | 24 27 00 00
- 3 | (0,3) | 12 | f | f | 25 27 00 00
- 4 | (0,4) | 12 | f | f | 26 27 00 00
- 5 | (0,5) | 12 | f | f | 27 27 00 00
- 6 | (0,6) | 12 | f | f | 28 27 00 00
- 7 | (0,7) | 12 | f | f | 29 27 00 00
- 8 | (0,8) | 12 | f | f | 2a 27 00 00
+ test=# SELECT itemoffset, ctid, itemlen, nulls, vars, data, dead, htid, tids[0:2] AS some_tids
+ FROM bt_page_items(get_raw_page('tenk2_hundred', 5));
+ itemoffset | ctid | itemlen | nulls | vars | data | dead | htid | some_tids
+------------+-----------+---------+-------+------+-------------------------+------+--------+---------------------
+ 1 | (16,1) | 16 | f | f | 30 00 00 00 00 00 00 00 | | |
+ 2 | (16,8292) | 616 | f | f | 24 00 00 00 00 00 00 00 | f | (1,6) | {"(1,6)","(10,22)"}
+ 3 | (16,8292) | 616 | f | f | 25 00 00 00 00 00 00 00 | f | (1,18) | {"(1,18)","(4,22)"}
+ 4 | (16,8292) | 616 | f | f | 26 00 00 00 00 00 00 00 | f | (4,18) | {"(4,18)","(6,17)"}
+ 5 | (16,8292) | 616 | f | f | 27 00 00 00 00 00 00 00 | f | (1,2) | {"(1,2)","(1,19)"}
+ 6 | (16,8292) | 616 | f | f | 28 00 00 00 00 00 00 00 | f | (2,24) | {"(2,24)","(4,11)"}
+ 7 | (16,8292) | 616 | f | f | 29 00 00 00 00 00 00 00 | f | (2,17) | {"(2,17)","(11,2)"}
+ 8 | (16,8292) | 616 | f | f | 2a 00 00 00 00 00 00 00 | f | (0,25) | {"(0,25)","(3,20)"}
</screen>
All the other details are the same as explained in the previous item.
</para>
--
2.17.1
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-03-01 18:24 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-03-01 18:24 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-committers@lists.postgresql.org
Peter Geoghegan <pg@bowt.ie> writes:
> Add deduplication to nbtree.
Coverity isn't very happy with the coding in _bt_update_posting():
*** CID 1460433: Memory - corruptions (ARRAY_VS_SINGLETON)
/srv/coverity/git/pgsql-git/postgresql/src/backend/access/nbtree/nbtdedup.c: 723 in _bt_update_posting()
717 {
718 if (d < vacposting->ndeletedtids && vacposting->deletetids[d] == i)
719 {
720 d++;
721 continue;
722 }
>>> CID 1460433: Memory - corruptions (ARRAY_VS_SINGLETON)
>>> Using "htids" as an array. This might corrupt or misinterpret adjacent memory locations.
723 htids[ui++] = *BTreeTupleGetPostingN(origtuple, i);
724 }
725 Assert(ui == nhtids);
726 Assert(d == vacposting->ndeletedtids);
727 Assert(nhtids == 1 || _bt_posting_valid(itup));
I can see its point: asserting after the fact that you didn't clobber
memory isn't a terribly safe coding method, especially in a production
build where you won't even have the asserts. Not sure if there's a
better way though.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-03-01 18:57 ` Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 22+ messages in thread
From: Peter Geoghegan @ 2020-03-01 18:57 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Sun, Mar 1, 2020 at 10:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I can see its point: asserting after the fact that you didn't clobber
> memory isn't a terribly safe coding method, especially in a production
> build where you won't even have the asserts. Not sure if there's a
> better way though.
I found it slightly more elegant to treat itup->t_tid as a degenerate
1 element posting list here, but I'm not particularly attached to that
approach. The loop is only truly necessary when dealing with a posting
list tuple.
Do you think that _bt_update_posting() should avoid this loop when
itup is just a plain tuple, that lacks a posting list? I can do it
that way if you prefer.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-03-01 19:29 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-03-01 19:29 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
Peter Geoghegan <pg@bowt.ie> writes:
> On Sun, Mar 1, 2020 at 10:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I can see its point: asserting after the fact that you didn't clobber
>> memory isn't a terribly safe coding method, especially in a production
>> build where you won't even have the asserts. Not sure if there's a
>> better way though.
> I found it slightly more elegant to treat itup->t_tid as a degenerate
> 1 element posting list here, but I'm not particularly attached to that
> approach. The loop is only truly necessary when dealing with a posting
> list tuple.
> Do you think that _bt_update_posting() should avoid this loop when
> itup is just a plain tuple, that lacks a posting list? I can do it
> that way if you prefer.
Hm. That would probably be enough to shut up Coverity, but I'm unsure
whether it'd really be an improvement from the legibility and safety
viewpoints. Do you want to try coding it that way and see what it
comes out like?
Note that we do have the ability to just dismiss the Coverity complaint,
if we decide that there's no better way to write the function. But
it's usually worth looking to see if it could be written more cleanly.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-03-01 19:42 ` Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 22+ messages in thread
From: Peter Geoghegan @ 2020-03-01 19:42 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Sun, Mar 1, 2020 at 11:29 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hm. That would probably be enough to shut up Coverity, but I'm unsure
> whether it'd really be an improvement from the legibility and safety
> viewpoints.
I noticed that _bt_update_posting() behaves as if the origtuple might
not be a posting list tuple at the point that keysize is calculated,
despite generally depending on it being a posting list tuple (which it
asserts by way of its "_bt_posting_valid(origtuple)" assertion). The
final tuple might not be a posting list, but the original one must be
(if it isn't, then nbtree VACUUM should be deleting it outright in the
traditional way, rather than updating it). I should fix that, either
way.
> Do you want to try coding it that way and see what it
> comes out like?
Sure.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-03-01 21:37 ` Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 22+ messages in thread
From: Peter Geoghegan @ 2020-03-01 21:37 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Sun, Mar 1, 2020 at 11:42 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > Do you want to try coding it that way and see what it
> > comes out like?
>
> Sure.
Attached patch shows how this could work. I prefer my original
approach, but I can see the argument for doing it this way.
If we keep my original approach, we should still add a new
"ItemPointerIsValid(&itup->t_tid)" assertion that covers the plain
tupe case in a way that mirrors the current "_bt_posting_valid(itup)"
assert.
--
Peter Geoghegan
Attachments:
[application/octet-stream] 0001-Restructure-_bt_update_posting.patch (2.3K, ../../CAH2-Wz=Tz_e8e39qTs3Mt78P6XD9s3u7TdOUVrYZg2eTEgfB0w@mail.gmail.com/2-0001-Restructure-_bt_update_posting.patch)
download | inline diff:
From a6048b4e8b8f84e83884c272e46b76943460bca7 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Sun, 1 Mar 2020 13:31:16 -0800
Subject: [PATCH] Restructure _bt_update_posting().
---
src/backend/access/nbtree/nbtdedup.c | 47 ++++++++++++++++++----------
1 file changed, 30 insertions(+), 17 deletions(-)
diff --git a/src/backend/access/nbtree/nbtdedup.c b/src/backend/access/nbtree/nbtdedup.c
index 3175d6fdd1..a52effe078 100644
--- a/src/backend/access/nbtree/nbtdedup.c
+++ b/src/backend/access/nbtree/nbtdedup.c
@@ -664,10 +664,7 @@ _bt_update_posting(BTVacuumPosting vacposting)
uint32 keysize,
newsize;
IndexTuple itup;
- int nhtids;
- int ui,
- d;
- ItemPointer htids;
+ int nhtids, d;
nhtids = BTreeTupleGetNPosting(origtuple) - vacposting->ndeletedtids;
@@ -696,31 +693,47 @@ _bt_update_posting(BTVacuumPosting vacposting)
if (nhtids > 1)
{
+ ItemPointer htids;
+ int ui;
+
/* Form posting list tuple */
BTreeTupleSetPosting(itup, nhtids, keysize);
htids = BTreeTupleGetPosting(itup);
+
+ ui = 0;
+ d = 0;
+ for (int i = 0; i < BTreeTupleGetNPosting(origtuple); i++)
+ {
+ if (d < vacposting->ndeletedtids && vacposting->deletetids[d] == i)
+ {
+ d++;
+ continue;
+ }
+ htids[ui++] = *BTreeTupleGetPostingN(origtuple, i);
+ }
+ Assert(ui == nhtids);
+ Assert(d == vacposting->ndeletedtids);
+ Assert(_bt_posting_valid(itup));
}
else
{
/* Form standard non-pivot tuple */
itup->t_info &= ~INDEX_ALT_TID_MASK;
- htids = &itup->t_tid;
- }
- ui = 0;
- d = 0;
- for (int i = 0; i < BTreeTupleGetNPosting(origtuple); i++)
- {
- if (d < vacposting->ndeletedtids && vacposting->deletetids[d] == i)
+ Assert(BTreeTupleGetNPosting(origtuple) == vacposting->ndeletedtids + 1);
+ d = 0;
+ for (int i = 0; i < BTreeTupleGetNPosting(origtuple); i++)
{
- d++;
- continue;
+ if (d < vacposting->ndeletedtids && vacposting->deletetids[d] == i)
+ {
+ d++;
+ continue;
+ }
+ itup->t_tid = *BTreeTupleGetPostingN(origtuple, i);
+ break;
}
- htids[ui++] = *BTreeTupleGetPostingN(origtuple, i);
+ Assert(ItemPointerIsValid(&itup->t_tid));
}
- Assert(ui == nhtids);
- Assert(d == vacposting->ndeletedtids);
- Assert(nhtids == 1 || _bt_posting_valid(itup));
/* vacposting arg's itup will now point to updated version */
vacposting->itup = itup;
--
2.17.1
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-03-01 22:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 23:01 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-03-01 22:14 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
Peter Geoghegan <pg@bowt.ie> writes:
> Attached patch shows how this could work. I prefer my original
> approach, but I can see the argument for doing it this way.
This does seem a bit duplicative ... and shouldn't both code paths
include a final "Assert(d == vacposting->ndeletedtids)"? So maybe
we're better off just rejecting the Coverity complaint.
> If we keep my original approach, we should still add a new
> "ItemPointerIsValid(&itup->t_tid)" assertion that covers the plain
> tupe case in a way that mirrors the current "_bt_posting_valid(itup)"
> assert.
Another thing that maybe bears closer scrutiny is the size calculation.
It seems a bit confused as to whether the offset of the posting list
within the tuple, or the total tuple size, or both, needs to be
MAXALIGN'd.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-03-01 23:01 ` Peter Geoghegan <pg@bowt.ie>
2020-03-02 00:09 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 22+ messages in thread
From: Peter Geoghegan @ 2020-03-01 23:01 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Sun, Mar 1, 2020 at 2:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Attached patch shows how this could work. I prefer my original
> > approach, but I can see the argument for doing it this way.
>
> This does seem a bit duplicative ... and shouldn't both code paths
> include a final "Assert(d == vacposting->ndeletedtids)"?
No, because the "nhtids == 1" loop has a "break" when the first and
only TID that we need to keep around is hit.
> Another thing that maybe bears closer scrutiny is the size calculation.
> It seems a bit confused as to whether the offset of the posting list
> within the tuple, or the total tuple size, or both, needs to be
> MAXALIGN'd.
That's not quite true in my opinion. BTreeTupleSetPosting() has an
Assert() of its own about alignment. ISTM that it's reasonable for
_bt_update_posting() to assume that BTreeTupleGetPostingOffset() will
return a MAXALIGN()'d offset. Note also that _bt_form_posting() is
explicit about what it expects around alignment. This is
_bt_update_posting()'s sibling function. The _bt_update_posting()
calculation is explicitly documented as being derived from the one in
_bt_form_posting().
I am happy to add parallel-to-_bt_form_posting() assertions about
alignment to _bt_form_posting(), to nail it down completely. Plus I'll
add the assertion I suggested already. Once I commit a patch with
these two new assertions, I think that we can consider the matter
resolved. Does that seem reasonable to you?
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 23:01 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-03-02 00:09 ` Peter Geoghegan <pg@bowt.ie>
2020-03-02 04:06 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-29 22:15 ` Re: pgsql: Add deduplication to nbtree. Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 22+ messages in thread
From: Peter Geoghegan @ 2020-03-02 00:09 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Sun, Mar 1, 2020 at 3:01 PM Peter Geoghegan <pg@bowt.ie> wrote:
> I am happy to add parallel-to-_bt_form_posting() assertions about
> alignment to _bt_form_posting(), to nail it down completely. Plus I'll
> add the assertion I suggested already. Once I commit a patch with
> these two new assertions, I think that we can consider the matter
> resolved. Does that seem reasonable to you?
I was thinking of the approach taken in the attached patch. It more or
less copies over the assertions from _bt_form_posting() that did not
appear in _bt_update_posting().
--
Peter Geoghegan
Attachments:
[application/octet-stream] 0001-Add-assertions-to-_bt_update_posting.patch (1.1K, ../../CAH2-WznRH9zmUn3tthVgHp3KDNvqCEL-E2u+xTgQYeT2xtfHqw@mail.gmail.com/2-0001-Add-assertions-to-_bt_update_posting.patch)
download | inline diff:
From 383640c9a3a728c2f9752f1acd958ab90adf37b5 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Sun, 1 Mar 2020 16:03:25 -0800
Subject: [PATCH] Add assertions to _bt_update_posting().
---
src/backend/access/nbtree/nbtdedup.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/access/nbtree/nbtdedup.c b/src/backend/access/nbtree/nbtdedup.c
index 3175d6fdd1..2434ce134b 100644
--- a/src/backend/access/nbtree/nbtdedup.c
+++ b/src/backend/access/nbtree/nbtdedup.c
@@ -688,6 +688,9 @@ _bt_update_posting(BTVacuumPosting vacposting)
else
newsize = keysize;
+ Assert(newsize <= INDEX_SIZE_MASK);
+ Assert(newsize == MAXALIGN(newsize));
+
/* Allocate memory using palloc0() (matches index_form_tuple()) */
itup = palloc0(newsize);
memcpy(itup, origtuple, keysize);
@@ -721,6 +724,7 @@ _bt_update_posting(BTVacuumPosting vacposting)
Assert(ui == nhtids);
Assert(d == vacposting->ndeletedtids);
Assert(nhtids == 1 || _bt_posting_valid(itup));
+ Assert(nhtids > 1 || ItemPointerIsValid(&itup->t_tid));
/* vacposting arg's itup will now point to updated version */
vacposting->itup = itup;
--
2.17.1
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 23:01 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 00:09 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-03-02 04:06 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-02 16:08 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
1 sibling, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-03-02 04:06 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
Peter Geoghegan <pg@bowt.ie> writes:
> On Sun, Mar 1, 2020 at 3:01 PM Peter Geoghegan <pg@bowt.ie> wrote:
>> I am happy to add parallel-to-_bt_form_posting() assertions about
>> alignment to _bt_form_posting(), to nail it down completely. Plus I'll
>> add the assertion I suggested already. Once I commit a patch with
>> these two new assertions, I think that we can consider the matter
>> resolved. Does that seem reasonable to you?
> I was thinking of the approach taken in the attached patch. It more or
> less copies over the assertions from _bt_form_posting() that did not
> appear in _bt_update_posting().
OK by me.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 23:01 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 00:09 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 04:06 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-03-02 16:08 ` Peter Geoghegan <pg@bowt.ie>
2020-03-02 17:47 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 22+ messages in thread
From: Peter Geoghegan @ 2020-03-02 16:08 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Sun, Mar 1, 2020 at 8:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I was thinking of the approach taken in the attached patch. It more or
> > less copies over the assertions from _bt_form_posting() that did not
> > appear in _bt_update_posting().
>
> OK by me.
Pushed. Thanks.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 23:01 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 00:09 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 04:06 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-02 16:08 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-03-02 17:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-02 18:29 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2020-03-02 17:47 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
Another issue I just noticed is that src/tools/pginclude/cpluspluscheck
complains thusly:
./src/include/access/nbtree.h: In function 'void BTreeTupleSetPosting(IndexTupleData*, int, int)':
./src/include/access/nbtree.h:384: warning: comparison between signed and unsigned integer expressions
I suppose this can be silenced with an appropriate cast, and doing so
would seem like a good idea.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 23:01 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 00:09 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 04:06 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-02 16:08 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 17:47 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-03-02 18:29 ` Peter Geoghegan <pg@bowt.ie>
0 siblings, 0 replies; 22+ messages in thread
From: Peter Geoghegan @ 2020-03-02 18:29 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Mon, Mar 2, 2020 at 9:47 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I suppose this can be silenced with an appropriate cast, and doing so
> would seem like a good idea.
I pushed a commit that silenced the cpluspluscheck warning just now.
Thanks
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 23:01 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 00:09 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-03-29 22:15 ` Andres Freund <andres@anarazel.de>
2020-03-29 22:19 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
1 sibling, 1 reply; 22+ messages in thread
From: Andres Freund @ 2020-03-29 22:15 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-committers <pgsql-committers@lists.postgresql.org>
Hi,
On 2020-03-01 16:09:37 -0800, Peter Geoghegan wrote:
> On Sun, Mar 1, 2020 at 3:01 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > I am happy to add parallel-to-_bt_form_posting() assertions about
> > alignment to _bt_form_posting(), to nail it down completely. Plus I'll
> > add the assertion I suggested already. Once I commit a patch with
> > these two new assertions, I think that we can consider the matter
> > resolved. Does that seem reasonable to you?
>
> I was thinking of the approach taken in the attached patch. It more or
> less copies over the assertions from _bt_form_posting() that did not
> appear in _bt_update_posting().
It seems, based on discussion on this thread and private inquiry to PG
that we're going to silence the warning, as there's not a good
resolution? I've for now marked the issue as a false positive, and added
a reference to this thread. But I think we should just mark it as
ignored in that case?
Is it perhaps possible to silence the warnign with somethign along the
lines of
Assert(nhtids + vacposting->ndeletedtids == BTreeTupleGetNPosting(origtuple))
I don't know this code, but it looks like that'd have to be true?
Perhaps that'd be enough to silence coverity too?
- Andres
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 23:01 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 00:09 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-29 22:15 ` Re: pgsql: Add deduplication to nbtree. Andres Freund <andres@anarazel.de>
@ 2020-03-29 22:19 ` Peter Geoghegan <pg@bowt.ie>
2020-03-29 22:41 ` Re: pgsql: Add deduplication to nbtree. Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 22+ messages in thread
From: Peter Geoghegan @ 2020-03-29 22:19 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-committers <pgsql-committers@lists.postgresql.org>
On Sun, Mar 29, 2020 at 3:15 PM Andres Freund <andres@anarazel.de> wrote:
> Is it perhaps possible to silence the warnign with somethign along the
> lines of
> Assert(nhtids + vacposting->ndeletedtids == BTreeTupleGetNPosting(origtuple))
> I don't know this code, but it looks like that'd have to be true?
> Perhaps that'd be enough to silence coverity too?
It would have to be true. It's a tautology. That is, the value of
nhtids comes from "vacposting->ndeletedtids" and
"BTreeTupleGetNPosting(origtuple)" anyway, and we don't mutate any of
that state in _bt_update_posting().
Wouldn't it at least be necessary to Assert() something about the
final tuple, and/or other work state?
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Re: pgsql: Add deduplication to nbtree. Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 23:01 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-02 00:09 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-03-29 22:15 ` Re: pgsql: Add deduplication to nbtree. Andres Freund <andres@anarazel.de>
2020-03-29 22:19 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-03-29 22:41 ` Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 22+ messages in thread
From: Andres Freund @ 2020-03-29 22:41 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-committers <pgsql-committers@lists.postgresql.org>
Hi,
On 2020-03-29 15:19:50 -0700, Peter Geoghegan wrote:
> On Sun, Mar 29, 2020 at 3:15 PM Andres Freund <andres@anarazel.de> wrote:
> > Is it perhaps possible to silence the warnign with somethign along the
> > lines of
> > Assert(nhtids + vacposting->ndeletedtids == BTreeTupleGetNPosting(origtuple))
> > I don't know this code, but it looks like that'd have to be true?
> > Perhaps that'd be enough to silence coverity too?
>
> It would have to be true. It's a tautology. That is, the value of
> nhtids comes from "vacposting->ndeletedtids" and
> "BTreeTupleGetNPosting(origtuple)" anyway, and we don't mutate any of
> that state in _bt_update_posting().
Right. It doesn't look like coverity understands that currently though.
> Wouldn't it at least be necessary to Assert() something about the
> final tuple, and/or other work state?
I don't see why? The assert might not help, but if it does, I don't
think it'd need more than that check? It depends a bit on what
coverity's precise logic here is. If its ARRAY_VS_SINGLETON checker
allows index (i.e. ui) 0 but not 1, then I think the suggested assert
could help it recognize that that's unreachable.
After staring more at the coverity trace, it looks however like
ARRAY_VS_SINGLETON might not allow any array like access, not even 0?
Seems like it assumes that BTreeTupleGetNPosting(origtuple) is at least
two, and that in the first loop d < vacposting->ndeletedtids and
vacposting->deletetids[d] == i are true, but in the second iteration
assumes d < vacposting->ndeletedtids is true but
vacposting->deletetids[d] == i. Since it doesn't record a third
iteration, it seems like it ought to see ui == 0 at that point.
I'll mark it as ignored.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-04-30 18:33 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2020-04-30 18:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2 siblings, 1 reply; 22+ messages in thread
From: Peter Eisentraut @ 2020-04-30 18:33 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; pgsql-committers@lists.postgresql.org
On 2020-02-26 22:06, Peter Geoghegan wrote:
> Add deduplication to nbtree.
AddressSanitizer has a use-after-scope complaint related to this patch.
The following change fixes it:
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index a70b64d964..8246ab4f18 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -1289,6 +1289,7 @@ _bt_insertonpg(Relation rel,
xl_btree_metadata xlmeta;
uint8 xlinfo;
XLogRecPtr recptr;
+ uint16 upostingoff;
xlrec.offnum = newitemoff;
@@ -1354,7 +1355,7 @@ _bt_insertonpg(Relation rel,
* must reconstruct final itup (as well as nposting) using
* _bt_swap_posting().
*/
- uint16 upostingoff = postingoff;
+ upostingoff = postingoff;
XLogRegisterBufData(0, (char *) &upostingoff, sizeof(uint16));
XLogRegisterBufData(0, (char *) origitup,
Please check.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-04-30 18:33 ` Re: pgsql: Add deduplication to nbtree. Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
@ 2020-04-30 18:37 ` Peter Geoghegan <pg@bowt.ie>
2020-04-30 19:33 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 22+ messages in thread
From: Peter Geoghegan @ 2020-04-30 18:37 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Thu, Apr 30, 2020 at 11:33 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> AddressSanitizer has a use-after-scope complaint related to this patch.
>
> The following change fixes it:
Your fix looks good to me. Please go ahead and commit it.
Thanks!
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: pgsql: Add deduplication to nbtree.
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-04-30 18:33 ` Re: pgsql: Add deduplication to nbtree. Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2020-04-30 18:37 ` Re: pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
@ 2020-04-30 19:33 ` Peter Geoghegan <pg@bowt.ie>
0 siblings, 0 replies; 22+ messages in thread
From: Peter Geoghegan @ 2020-04-30 19:33 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Thu, Apr 30, 2020 at 11:37 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > The following change fixes it:
>
> Your fix looks good to me. Please go ahead and commit it.
Actually, never mind. I just pushed your fix myself a moment ago.
Thanks again
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 22+ messages in thread
end of thread, other threads:[~2020-04-30 19:33 UTC | newest]
Thread overview: 22+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26 21:06 pgsql: Add deduplication to nbtree. Peter Geoghegan <pg@bowt.ie>
2020-02-27 17:25 ` Laurenz Albe <laurenz.albe@cybertec.at>
2020-02-27 17:26 ` Peter Geoghegan <pg@bowt.ie>
2020-02-29 02:37 ` Peter Geoghegan <pg@bowt.ie>
2020-03-01 18:24 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 18:57 ` Peter Geoghegan <pg@bowt.ie>
2020-03-01 19:29 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 19:42 ` Peter Geoghegan <pg@bowt.ie>
2020-03-01 21:37 ` Peter Geoghegan <pg@bowt.ie>
2020-03-01 22:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-01 23:01 ` Peter Geoghegan <pg@bowt.ie>
2020-03-02 00:09 ` Peter Geoghegan <pg@bowt.ie>
2020-03-02 04:06 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-02 16:08 ` Peter Geoghegan <pg@bowt.ie>
2020-03-02 17:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-02 18:29 ` Peter Geoghegan <pg@bowt.ie>
2020-03-29 22:15 ` Andres Freund <andres@anarazel.de>
2020-03-29 22:19 ` Peter Geoghegan <pg@bowt.ie>
2020-03-29 22:41 ` Andres Freund <andres@anarazel.de>
2020-04-30 18:33 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2020-04-30 18:37 ` Peter Geoghegan <pg@bowt.ie>
2020-04-30 19:33 ` Peter Geoghegan <pg@bowt.ie>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox