public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4] pageinspect: Change block number arguments to bigint
5+ messages / 5 participants
[nested] [flat]
* [PATCH v4] pageinspect: Change block number arguments to bigint
@ 2021-01-13 08:12 Peter Eisentraut <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Peter Eisentraut @ 2021-01-13 08:12 UTC (permalink / raw)
Block numbers are 32-bit unsigned integers. Therefore, the smallest
SQL integer type that they can fit in is bigint. However, in the
pageinspect module, most input and output parameters dealing with
block numbers were declared as int. The behavior with block numbers
larger than a signed 32-bit integer was therefore dubious. Change
these arguments to type bigint and add some more explicit error
checking on the block range.
(Other contrib modules appear to do this correctly already.)
Since we are changing argument types of existing functions, in order
to not misbehave if the binary is updated before the extension is
updated, we need to create new C symbols for the entry points, similar
to how it's done in other extensions as well.
Reported-by: Ashutosh Bapat <[email protected]>
Reviewed-by: Alvaro Herrera <[email protected]>
Discussion: https://www.postgresql.org/message-id/flat/[email protected]...
---
contrib/pageinspect/Makefile | 5 +-
contrib/pageinspect/brinfuncs.c | 13 ++-
contrib/pageinspect/btreefuncs.c | 76 +++++++++++++----
contrib/pageinspect/expected/gin.out | 1 +
.../pageinspect/expected/oldextversions.out | 40 +++++++++
contrib/pageinspect/hashfuncs.c | 11 ++-
contrib/pageinspect/pageinspect--1.8--1.9.sql | 81 +++++++++++++++++++
contrib/pageinspect/pageinspect.control | 2 +-
contrib/pageinspect/pageinspect.h | 9 +++
contrib/pageinspect/rawpage.c | 75 ++++++++++++++++-
contrib/pageinspect/sql/gin.sql | 2 +
contrib/pageinspect/sql/oldextversions.sql | 20 +++++
doc/src/sgml/pageinspect.sgml | 12 +--
13 files changed, 314 insertions(+), 33 deletions(-)
create mode 100644 contrib/pageinspect/expected/oldextversions.out
create mode 100644 contrib/pageinspect/pageinspect--1.8--1.9.sql
create mode 100644 contrib/pageinspect/sql/oldextversions.sql
diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index d9d8177116..b78ffceaff 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -12,14 +12,15 @@ OBJS = \
rawpage.o
EXTENSION = pageinspect
-DATA = pageinspect--1.7--1.8.sql pageinspect--1.6--1.7.sql \
+DATA = pageinspect--1.8--1.9.sql \
+ pageinspect--1.7--1.8.sql pageinspect--1.6--1.7.sql \
pageinspect--1.5.sql pageinspect--1.5--1.6.sql \
pageinspect--1.4--1.5.sql pageinspect--1.3--1.4.sql \
pageinspect--1.2--1.3.sql pageinspect--1.1--1.2.sql \
pageinspect--1.0--1.1.sql
PGFILEDESC = "pageinspect - functions to inspect contents of database pages"
-REGRESS = page btree brin gin hash checksum
+REGRESS = page btree brin gin hash checksum oldextversions
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index e872f65f01..0e3c2deb66 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -252,7 +252,18 @@ brin_page_items(PG_FUNCTION_ARGS)
int att = attno - 1;
values[0] = UInt16GetDatum(offset);
- values[1] = UInt32GetDatum(dtup->bt_blkno);
+ switch (TupleDescAttr(tupdesc, 1)->atttypid)
+ {
+ case INT8OID:
+ values[1] = Int64GetDatum((int64) dtup->bt_blkno);
+ break;
+ case INT4OID:
+ /* support for old extension version */
+ values[1] = UInt32GetDatum(dtup->bt_blkno);
+ break;
+ default:
+ elog(ERROR, "incorrect output types");
+ }
values[2] = UInt16GetDatum(attno);
values[3] = BoolGetDatum(dtup->bt_columns[att].bv_allnulls);
values[4] = BoolGetDatum(dtup->bt_columns[att].bv_hasnulls);
diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index 445605db58..8bb180bbbe 100644
--- a/contrib/pageinspect/btreefuncs.c
+++ b/contrib/pageinspect/btreefuncs.c
@@ -41,8 +41,10 @@
#include "utils/varlena.h"
PG_FUNCTION_INFO_V1(bt_metap);
+PG_FUNCTION_INFO_V1(bt_page_items_1_9);
PG_FUNCTION_INFO_V1(bt_page_items);
PG_FUNCTION_INFO_V1(bt_page_items_bytea);
+PG_FUNCTION_INFO_V1(bt_page_stats_1_9);
PG_FUNCTION_INFO_V1(bt_page_stats);
#define IS_INDEX(r) ((r)->rd_rel->relkind == RELKIND_INDEX)
@@ -160,11 +162,11 @@ GetBTPageStatistics(BlockNumber blkno, Buffer buffer, BTPageStat *stat)
* Usage: SELECT * FROM bt_page_stats('t1_pkey', 1);
* -----------------------------------------------
*/
-Datum
-bt_page_stats(PG_FUNCTION_ARGS)
+static Datum
+bt_page_stats_internal(PG_FUNCTION_ARGS, enum pageinspect_version ext_version)
{
text *relname = PG_GETARG_TEXT_PP(0);
- uint32 blkno = PG_GETARG_UINT32(1);
+ int64 blkno = (ext_version == PAGEINSPECT_V1_8 ? PG_GETARG_UINT32(1) : PG_GETARG_INT64(1));
Buffer buffer;
Relation rel;
RangeVar *relrv;
@@ -197,8 +199,15 @@ bt_page_stats(PG_FUNCTION_ARGS)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary tables of other sessions")));
+ if (blkno < 0 || blkno > MaxBlockNumber)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number")));
+
if (blkno == 0)
- elog(ERROR, "block 0 is a meta page");
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("block 0 is a meta page")));
CHECK_RELATION_BLOCK_RANGE(rel, blkno);
@@ -219,16 +228,16 @@ bt_page_stats(PG_FUNCTION_ARGS)
elog(ERROR, "return type must be a row type");
j = 0;
- values[j++] = psprintf("%d", stat.blkno);
+ values[j++] = psprintf("%u", stat.blkno);
values[j++] = psprintf("%c", stat.type);
- values[j++] = psprintf("%d", stat.live_items);
- values[j++] = psprintf("%d", stat.dead_items);
- values[j++] = psprintf("%d", stat.avg_item_size);
- values[j++] = psprintf("%d", stat.page_size);
- values[j++] = psprintf("%d", stat.free_size);
- values[j++] = psprintf("%d", stat.btpo_prev);
- values[j++] = psprintf("%d", stat.btpo_next);
- values[j++] = psprintf("%d", (stat.type == 'd') ? stat.btpo.xact : stat.btpo.level);
+ values[j++] = psprintf("%u", stat.live_items);
+ values[j++] = psprintf("%u", stat.dead_items);
+ values[j++] = psprintf("%u", stat.avg_item_size);
+ values[j++] = psprintf("%u", stat.page_size);
+ values[j++] = psprintf("%u", stat.free_size);
+ values[j++] = psprintf("%u", stat.btpo_prev);
+ values[j++] = psprintf("%u", stat.btpo_next);
+ values[j++] = psprintf("%u", (stat.type == 'd') ? stat.btpo.xact : stat.btpo.level);
values[j++] = psprintf("%d", stat.btpo_flags);
tuple = BuildTupleFromCStrings(TupleDescGetAttInMetadata(tupleDesc),
@@ -239,6 +248,19 @@ bt_page_stats(PG_FUNCTION_ARGS)
PG_RETURN_DATUM(result);
}
+Datum
+bt_page_stats_1_9(PG_FUNCTION_ARGS)
+{
+ return bt_page_stats_internal(fcinfo, PAGEINSPECT_V1_9);
+}
+
+/* entry point for old extension version */
+Datum
+bt_page_stats(PG_FUNCTION_ARGS)
+{
+ return bt_page_stats_internal(fcinfo, PAGEINSPECT_V1_8);
+}
+
/*
* cross-call data structure for SRF
@@ -405,11 +427,11 @@ bt_page_print_tuples(struct user_args *uargs)
* Usage: SELECT * FROM bt_page_items('t1_pkey', 1);
*-------------------------------------------------------
*/
-Datum
-bt_page_items(PG_FUNCTION_ARGS)
+static Datum
+bt_page_items_internal(PG_FUNCTION_ARGS, enum pageinspect_version ext_version)
{
text *relname = PG_GETARG_TEXT_PP(0);
- uint32 blkno = PG_GETARG_UINT32(1);
+ int64 blkno = (ext_version == PAGEINSPECT_V1_8 ? PG_GETARG_UINT32(1) : PG_GETARG_INT64(1));
Datum result;
FuncCallContext *fctx;
MemoryContext mctx;
@@ -447,8 +469,15 @@ bt_page_items(PG_FUNCTION_ARGS)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary tables of other sessions")));
+ if (blkno < 0 || blkno > MaxBlockNumber)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number")));
+
if (blkno == 0)
- elog(ERROR, "block 0 is a meta page");
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("block 0 is a meta page")));
CHECK_RELATION_BLOCK_RANGE(rel, blkno);
@@ -506,6 +535,19 @@ bt_page_items(PG_FUNCTION_ARGS)
SRF_RETURN_DONE(fctx);
}
+Datum
+bt_page_items_1_9(PG_FUNCTION_ARGS)
+{
+ return bt_page_items_internal(fcinfo, PAGEINSPECT_V1_9);
+}
+
+/* entry point for old extension version */
+Datum
+bt_page_items(PG_FUNCTION_ARGS)
+{
+ return bt_page_items_internal(fcinfo, PAGEINSPECT_V1_8);
+}
+
/*-------------------------------------------------------
* bt_page_items_bytea()
*
diff --git a/contrib/pageinspect/expected/gin.out b/contrib/pageinspect/expected/gin.out
index 82f63b23b1..ef7570b972 100644
--- a/contrib/pageinspect/expected/gin.out
+++ b/contrib/pageinspect/expected/gin.out
@@ -35,3 +35,4 @@ FROM gin_leafpage_items(get_raw_page('test1_y_idx',
-[ RECORD 1 ]
?column? | t
+DROP TABLE test1;
diff --git a/contrib/pageinspect/expected/oldextversions.out b/contrib/pageinspect/expected/oldextversions.out
new file mode 100644
index 0000000000..04dc7f8640
--- /dev/null
+++ b/contrib/pageinspect/expected/oldextversions.out
@@ -0,0 +1,40 @@
+-- test old extension version entry points
+DROP EXTENSION pageinspect;
+CREATE EXTENSION pageinspect VERSION '1.8';
+CREATE TABLE test1 (a int8, b text);
+INSERT INTO test1 VALUES (72057594037927937, 'text');
+CREATE INDEX test1_a_idx ON test1 USING btree (a);
+-- from page.sql
+SELECT octet_length(get_raw_page('test1', 0)) AS main_0;
+ main_0
+--------
+ 8192
+(1 row)
+
+SELECT octet_length(get_raw_page('test1', 'main', 0)) AS main_0;
+ main_0
+--------
+ 8192
+(1 row)
+
+SELECT page_checksum(get_raw_page('test1', 0), 0) IS NOT NULL AS silly_checksum_test;
+ silly_checksum_test
+---------------------
+ t
+(1 row)
+
+-- from btree.sql
+SELECT * FROM bt_page_stats('test1_a_idx', 1);
+ blkno | type | live_items | dead_items | avg_item_size | page_size | free_size | btpo_prev | btpo_next | btpo | btpo_flags
+-------+------+------------+------------+---------------+-----------+-----------+-----------+-----------+------+------------
+ 1 | l | 1 | 0 | 16 | 8192 | 8128 | 0 | 0 | 0 | 3
+(1 row)
+
+SELECT * FROM bt_page_items('test1_a_idx', 1);
+ itemoffset | ctid | itemlen | nulls | vars | data | dead | htid | tids
+------------+-------+---------+-------+------+-------------------------+------+-------+------
+ 1 | (0,1) | 16 | f | f | 01 00 00 00 00 00 00 01 | f | (0,1) |
+(1 row)
+
+DROP TABLE test1;
+DROP EXTENSION pageinspect;
diff --git a/contrib/pageinspect/hashfuncs.c b/contrib/pageinspect/hashfuncs.c
index aa11ef396b..5934308751 100644
--- a/contrib/pageinspect/hashfuncs.c
+++ b/contrib/pageinspect/hashfuncs.c
@@ -390,7 +390,7 @@ Datum
hash_bitmap_info(PG_FUNCTION_ARGS)
{
Oid indexRelid = PG_GETARG_OID(0);
- uint64 ovflblkno = PG_GETARG_INT64(1);
+ int64 ovflblkno = PG_GETARG_INT64(1);
HashMetaPage metap;
Buffer metabuf,
mapbuf;
@@ -425,11 +425,16 @@ hash_bitmap_info(PG_FUNCTION_ARGS)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary tables of other sessions")));
+ if (ovflblkno < 0 || ovflblkno > MaxBlockNumber)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number")));
+
if (ovflblkno >= RelationGetNumberOfBlocks(indexRel))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("block number " UINT64_FORMAT " is out of range for relation \"%s\"",
- ovflblkno, RelationGetRelationName(indexRel))));
+ errmsg("block number %llu is out of range for relation \"%s\"",
+ (long long unsigned) ovflblkno, RelationGetRelationName(indexRel))));
/* Read the metapage so we can determine which bitmap page to use */
metabuf = _hash_getbuf(indexRel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
diff --git a/contrib/pageinspect/pageinspect--1.8--1.9.sql b/contrib/pageinspect/pageinspect--1.8--1.9.sql
new file mode 100644
index 0000000000..46e8efc6d8
--- /dev/null
+++ b/contrib/pageinspect/pageinspect--1.8--1.9.sql
@@ -0,0 +1,81 @@
+/* contrib/pageinspect/pageinspect--1.8--1.9.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pageinspect UPDATE TO '1.9'" to load this file. \quit
+
+--
+-- get_raw_page()
+--
+DROP FUNCTION get_raw_page(text, int4);
+CREATE FUNCTION get_raw_page(text, int8)
+RETURNS bytea
+AS 'MODULE_PATHNAME', 'get_raw_page_1_9'
+LANGUAGE C STRICT PARALLEL SAFE;
+
+DROP FUNCTION get_raw_page(text, text, int4);
+CREATE FUNCTION get_raw_page(text, text, int8)
+RETURNS bytea
+AS 'MODULE_PATHNAME', 'get_raw_page_fork_1_9'
+LANGUAGE C STRICT PARALLEL SAFE;
+
+--
+-- page_checksum()
+--
+DROP FUNCTION page_checksum(IN page bytea, IN blkno int4);
+CREATE FUNCTION page_checksum(IN page bytea, IN blkno int8)
+RETURNS smallint
+AS 'MODULE_PATHNAME', 'page_checksum_1_9'
+LANGUAGE C STRICT PARALLEL SAFE;
+
+--
+-- bt_page_stats()
+--
+DROP FUNCTION bt_page_stats(text, int4);
+CREATE FUNCTION bt_page_stats(IN relname text, IN blkno int8,
+ OUT blkno int8,
+ OUT type "char",
+ OUT live_items int4,
+ OUT dead_items int4,
+ OUT avg_item_size int4,
+ OUT page_size int4,
+ OUT free_size int4,
+ OUT btpo_prev int8,
+ OUT btpo_next int8,
+ OUT btpo int4,
+ OUT btpo_flags int4)
+AS 'MODULE_PATHNAME', 'bt_page_stats_1_9'
+LANGUAGE C STRICT PARALLEL SAFE;
+
+--
+-- bt_page_items()
+--
+DROP FUNCTION bt_page_items(text, int4);
+CREATE FUNCTION bt_page_items(IN relname text, IN blkno int8,
+ 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_1_9'
+LANGUAGE C STRICT PARALLEL SAFE;
+
+--
+-- brin_page_items()
+--
+DROP FUNCTION brin_page_items(IN page bytea, IN index_oid regclass);
+CREATE FUNCTION brin_page_items(IN page bytea, IN index_oid regclass,
+ OUT itemoffset int,
+ OUT blknum int8,
+ OUT attnum int,
+ OUT allnulls bool,
+ OUT hasnulls bool,
+ OUT placeholder bool,
+ OUT value text)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'brin_page_items'
+LANGUAGE C STRICT PARALLEL SAFE;
diff --git a/contrib/pageinspect/pageinspect.control b/contrib/pageinspect/pageinspect.control
index f8cdf526c6..bd716769a1 100644
--- a/contrib/pageinspect/pageinspect.control
+++ b/contrib/pageinspect/pageinspect.control
@@ -1,5 +1,5 @@
# pageinspect extension
comment = 'inspect the contents of database pages at a low level'
-default_version = '1.8'
+default_version = '1.9'
module_pathname = '$libdir/pageinspect'
relocatable = true
diff --git a/contrib/pageinspect/pageinspect.h b/contrib/pageinspect/pageinspect.h
index c15bc23fe6..3812a3c233 100644
--- a/contrib/pageinspect/pageinspect.h
+++ b/contrib/pageinspect/pageinspect.h
@@ -15,6 +15,15 @@
#include "storage/bufpage.h"
+/*
+ * Extension version number, for supporting older extension versions' objects
+ */
+enum pageinspect_version
+{
+ PAGEINSPECT_V1_8,
+ PAGEINSPECT_V1_9,
+};
+
/* in rawpage.c */
extern Page get_page_from_raw(bytea *raw_page);
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index ae1dc41e05..9e9ee8a493 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -40,6 +40,28 @@ static bytea *get_raw_page_internal(text *relname, ForkNumber forknum,
*
* Returns a copy of a page from shared buffers as a bytea
*/
+PG_FUNCTION_INFO_V1(get_raw_page_1_9);
+
+Datum
+get_raw_page_1_9(PG_FUNCTION_ARGS)
+{
+ text *relname = PG_GETARG_TEXT_PP(0);
+ int64 blkno = PG_GETARG_INT64(1);
+ bytea *raw_page;
+
+ if (blkno < 0 || blkno > MaxBlockNumber)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number")));
+
+ raw_page = get_raw_page_internal(relname, MAIN_FORKNUM, blkno);
+
+ PG_RETURN_BYTEA_P(raw_page);
+}
+
+/*
+ * entry point for old extension version
+ */
PG_FUNCTION_INFO_V1(get_raw_page);
Datum
@@ -69,6 +91,32 @@ get_raw_page(PG_FUNCTION_ARGS)
*
* Same, for any fork
*/
+PG_FUNCTION_INFO_V1(get_raw_page_fork_1_9);
+
+Datum
+get_raw_page_fork_1_9(PG_FUNCTION_ARGS)
+{
+ text *relname = PG_GETARG_TEXT_PP(0);
+ text *forkname = PG_GETARG_TEXT_PP(1);
+ int64 blkno = PG_GETARG_INT64(2);
+ bytea *raw_page;
+ ForkNumber forknum;
+
+ forknum = forkname_to_number(text_to_cstring(forkname));
+
+ if (blkno < 0 || blkno > MaxBlockNumber)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number")));
+
+ raw_page = get_raw_page_internal(relname, forknum, blkno);
+
+ PG_RETURN_BYTEA_P(raw_page);
+}
+
+/*
+ * Entry point for old extension version
+ */
PG_FUNCTION_INFO_V1(get_raw_page_fork);
Datum
@@ -292,13 +340,14 @@ page_header(PG_FUNCTION_ARGS)
* Compute checksum of a raw page
*/
+PG_FUNCTION_INFO_V1(page_checksum_1_9);
PG_FUNCTION_INFO_V1(page_checksum);
-Datum
-page_checksum(PG_FUNCTION_ARGS)
+static Datum
+page_checksum_internal(PG_FUNCTION_ARGS, enum pageinspect_version ext_version)
{
bytea *raw_page = PG_GETARG_BYTEA_P(0);
- uint32 blkno = PG_GETARG_INT32(1);
+ int64 blkno = (ext_version == PAGEINSPECT_V1_8 ? PG_GETARG_UINT32(1) : PG_GETARG_INT64(1));
int raw_page_size;
PageHeader page;
@@ -307,6 +356,11 @@ page_checksum(PG_FUNCTION_ARGS)
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to use raw page functions")));
+ if (blkno < 0 || blkno > MaxBlockNumber)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number")));
+
raw_page_size = VARSIZE(raw_page) - VARHDRSZ;
/*
@@ -321,3 +375,18 @@ page_checksum(PG_FUNCTION_ARGS)
PG_RETURN_INT16(pg_checksum_page((char *) page, blkno));
}
+
+Datum
+page_checksum_1_9(PG_FUNCTION_ARGS)
+{
+ return page_checksum_internal(fcinfo, PAGEINSPECT_V1_9);
+}
+
+/*
+ * Entry point for old extension version
+ */
+Datum
+page_checksum(PG_FUNCTION_ARGS)
+{
+ return page_checksum_internal(fcinfo, PAGEINSPECT_V1_8);
+}
diff --git a/contrib/pageinspect/sql/gin.sql b/contrib/pageinspect/sql/gin.sql
index d516ed3cbd..423f5c5749 100644
--- a/contrib/pageinspect/sql/gin.sql
+++ b/contrib/pageinspect/sql/gin.sql
@@ -17,3 +17,5 @@ CREATE INDEX test1_y_idx ON test1 USING gin (y) WITH (fastupdate = off);
FROM gin_leafpage_items(get_raw_page('test1_y_idx',
(pg_relation_size('test1_y_idx') /
current_setting('block_size')::bigint)::int - 1));
+
+DROP TABLE test1;
diff --git a/contrib/pageinspect/sql/oldextversions.sql b/contrib/pageinspect/sql/oldextversions.sql
new file mode 100644
index 0000000000..78e08f40e8
--- /dev/null
+++ b/contrib/pageinspect/sql/oldextversions.sql
@@ -0,0 +1,20 @@
+-- test old extension version entry points
+
+DROP EXTENSION pageinspect;
+CREATE EXTENSION pageinspect VERSION '1.8';
+
+CREATE TABLE test1 (a int8, b text);
+INSERT INTO test1 VALUES (72057594037927937, 'text');
+CREATE INDEX test1_a_idx ON test1 USING btree (a);
+
+-- from page.sql
+SELECT octet_length(get_raw_page('test1', 0)) AS main_0;
+SELECT octet_length(get_raw_page('test1', 'main', 0)) AS main_0;
+SELECT page_checksum(get_raw_page('test1', 0), 0) IS NOT NULL AS silly_checksum_test;
+
+-- from btree.sql
+SELECT * FROM bt_page_stats('test1_a_idx', 1);
+SELECT * FROM bt_page_items('test1_a_idx', 1);
+
+DROP TABLE test1;
+DROP EXTENSION pageinspect;
diff --git a/doc/src/sgml/pageinspect.sgml b/doc/src/sgml/pageinspect.sgml
index 687c3606ba..233c812345 100644
--- a/doc/src/sgml/pageinspect.sgml
+++ b/doc/src/sgml/pageinspect.sgml
@@ -19,7 +19,7 @@ <title>General Functions</title>
<variablelist>
<varlistentry>
<term>
- <function>get_raw_page(relname text, fork text, blkno int) returns bytea</function>
+ <function>get_raw_page(relname text, fork text, blkno bigint) returns bytea</function>
<indexterm>
<primary>get_raw_page</primary>
</indexterm>
@@ -40,7 +40,7 @@ <title>General Functions</title>
<varlistentry>
<term>
- <function>get_raw_page(relname text, blkno int) returns bytea</function>
+ <function>get_raw_page(relname text, blkno bigint) returns bytea</function>
</term>
<listitem>
@@ -91,7 +91,7 @@ <title>General Functions</title>
<varlistentry>
<term>
- <function>page_checksum(page bytea, blkno int4) returns smallint</function>
+ <function>page_checksum(page bytea, blkno bigint) returns smallint</function>
<indexterm>
<primary>page_checksum</primary>
</indexterm>
@@ -315,7 +315,7 @@ <title>B-Tree Functions</title>
<varlistentry>
<term>
- <function>bt_page_stats(relname text, blkno int) returns record</function>
+ <function>bt_page_stats(relname text, blkno bigint) returns record</function>
<indexterm>
<primary>bt_page_stats</primary>
</indexterm>
@@ -346,7 +346,7 @@ <title>B-Tree Functions</title>
<varlistentry>
<term>
- <function>bt_page_items(relname text, blkno int) returns setof record</function>
+ <function>bt_page_items(relname text, blkno bigint) returns setof record</function>
<indexterm>
<primary>bt_page_items</primary>
</indexterm>
@@ -756,7 +756,7 @@ <title>Hash Functions</title>
<varlistentry>
<term>
- <function>hash_bitmap_info(index oid, blkno int) returns record</function>
+ <function>hash_bitmap_info(index oid, blkno bigint) returns record</function>
<indexterm>
<primary>hash_bitmap_info</primary>
</indexterm>
base-commit: df10ac625c1672edf839ff59cfcac9dcc097515c
--
2.30.0
--------------FB0C47795AF1F8AA0A4706F6--
^ permalink raw reply [nested|flat] 5+ messages in thread
* Initial Schema Sync for Logical Replication
@ 2023-03-15 17:42 Kumar, Sachin <[email protected]>
0 siblings, 3 replies; 5+ messages in thread
From: Kumar, Sachin @ 2023-03-15 17:42 UTC (permalink / raw)
To: [email protected] <[email protected]>
Hi Everyone,
I am working on the initial schema sync for Logical replication. Currently, user have to
manually create a schema on subscriber side. Aim of this feature is to add an option in
create subscription, so that schema sync can be automatic. I am sharing Design Doc below,
but there are some corner cases where the design does not work. Please share your opinion
if design can be improved and we can get rid of corner cases. This design is loosely based
on Pglogical.
DDL replication is required for this feature.
(https://www.postgresql.org/message-id/flat/CAAD30U%2BpVmfKwUKy8cbZOnUXyguJ-uBNejwD75Kyo%3DOjdQGJ9g%4...)
SQL Changes:-
CREATE SUBSCRIPTION subscription_name
CONNECTION 'conninfo'
PUBLICATION publication_name [, ...]
[ WITH ( subscription_parameter [= value] [, ... ] ) ]
sync_initial_schema (enum) will be added to subscription_parameter.
It can have 3 values:-
TABLES, ALL , NONE (Default)
In ALL everything will be synced including global objects too.
Restrictions :- sync_initial_schema=ALL can only be used for publication with FOR ALL TABLES
Design:-
Publisher :-
Publisher have to implement `SHOW CREATE TABLE_NAME`, this table definition will be used by
subscriber to create exact schema of a table on the subscriber. One alternative to this can
be doing it on the subscriber side itself, we can create a function similar to
describeOneTableDetails and call it on the subscriber. We also need maintain same ownership
as of publisher.
It should also have turned on publication of DDL commands.
Subscriber :-
1. In CreateSubscription() when we create replication slot(walrcv_create_slot()), should
use CRS_EXPORT_SNAPSHOT, So that we can use this snapshot later in the pg_dump.
2. Now we can call pg_dump with above snapshot from CreateSubscription. This is inside
opts.connect && opts.create_slot if statement. If we fail in this step we have to drop
the replication slot and create a new one again. Because we need snapshot and creating a
replication slot is a way to get snapshot. The reason for running pg_dump with above
snapshot is that we don't want execute DDLs in wal_logs to 2 times. With above snapshot we
get a state of database which is before the replication slot origin and any changes after
the snapshot will be in wal_logs.
We will save the pg_dump into a file (custom archive format). So pg_dump will be similar to
pg_dump --connection_string --schema_only --snapshot=xyz -Fc --file initSchema
If sync_initial_schema=TABLES we dont have to call pg_dump/restore at all. TableSync process
will take care of it.
3. If we have to sync global objects we need to call pg_dumpall --globals-only also. But pg_dumpall
does not support --snapshot option, So if user creates a new global object between creation
of replication slot and running pg_dumpall, that above global object will be created 2
times on subscriber , which will error out the Applier process.
4. walrcv_disconnect should be called after pg_dump is finished, otherwise snapshot will
not be valid.
5. Users will replication role cant not call pg_dump , So the replication user have to
superuser. This is a a major problem.
postgres=# create role s4 WITH LOGIN Replication;
CREATE ROLE
╭─sachin@DUB-1800550165 ~
╰─$ pg_dump postgres -s -U s4 1 ↵
pg_dump: error: query failed: ERROR: permission denied for table t1
pg_dump: detail: Query was: LOCK TABLE public.t1, public.t2 IN ACCESS SHARE MODE
6. pg_subscription_rel table column srsubstate will have one more state
SUBREL_STATE_CREATE 'c'. if sync_initial_schema is enabled we will set table_state to 'c'.
Above 6 steps will be done even if subscription is not enabled, but connect is true.
7. Leader Applier process should check if initSync file exist , if true then it should
call pg_restore. We are not using —pre-data and —post-data segment as it is used in
Pglogical, Because post_data works on table having data , but we will fill the data into
table on later stages. pg_restore can be called like this
pg_restore --connection_string -1 file_name
-1 option will execute every command inside of one transaction. If there is any error
everything will be rollbacked.
pg_restore should be called quite early in the Applier process code, before any tablesync
process can be created.
Instead of checking if file exist maybe pg_subscription table can be extended with column
SyncInitialSchema and applier process will check SyncInitialSchema == SYNC_PENDING
8. TableSync process should check the state of table , if it is SUBREL_STATE_CREATE it should
get the latest definition from the publisher and recreate the table. (We have to recreate
the table even if there are no changes). Then it should go into copy table mode as usual.
It might seem that TableSync is doing duplicate work already done by pg_restore. We are doing
it in this way because of concurrent DDLs and refresh publication command.
Concurrent DDL :-
User can execute a DDL command to table t1 at the same time when subscriber is trying to sync
it. pictorial representation https://imgur.com/a/ivrIEv8 [1]
In tablesync process, it makes a connection to the publisher and it sees the
table state which can be in future wrt to the publisher, which can introduce conflicts.
For example:-
CASE 1:- { Publisher removed the column b from the table t1 when subscriber was doing pg_restore
(or any point in concurrent DDL window described in picture [1] ), when tableSync
process will start transaction on the publisher it will see request data of table t1
including column b, which does not exist on the publisher.} So that is why tableSync process
asks for the latest definition.
If we say that we will delay tableSync worker till all the DDL related to table t1 is
applied by the applier process , we can still have a window when publisher issues a DDL
command just before tableSync starts its transaction, and therefore making tableSync and
publisher table definition incompatible (Thanks to Masahiko for pointing out this race
condition).
Applier process will skip all DDL/DMLs related to the table t1 and tableSync will apply those
in Catchup phase.
Although there is one issue what will happen to views/ or functions which depend on the table
. I think they should wait till table_state is > SUBREL_STATE_CREATE (means we have the latest
schema definition from the publisher).
There might be corner cases to this approach or maybe a better way to handle concurrent DDL
One simple solution might be to disallow DDLs on the publisher till all the schema is
synced and all tables have state >= SUBREL_STATE_DATASYNC (We can have CASE 1: issue ,
even with DDL replication, so we have to wait till all the tables have table_state
> SUBREL_STATE_DATASYNC). Which might be a big window for big databases.
Refresh publication :-
In refresh publication, subscriber does create a new replication slot hence , we can’t run
pg_dump with a snapshot which starts from origin(maybe this is not an issue at all). In this case
it makes more sense for tableSync worker to do schema sync.
If community is happy with above design, I can start working on prototype.
Credits :- This design is inspired by Pglogical. Also thanks to Zane, Masahiko, Amit for reviewing earlier designs
Regards
Sachin Kumar
Amazon Web Services
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Initial Schema Sync for Logical Replication
@ 2023-03-16 04:18 Peter Smith <[email protected]>
parent: Kumar, Sachin <[email protected]>
2 siblings, 0 replies; 5+ messages in thread
From: Peter Smith @ 2023-03-16 04:18 UTC (permalink / raw)
To: Kumar, Sachin <[email protected]>; +Cc: [email protected] <[email protected]>
Hi,
I have a couple of questions.
Q1.
What happens if the subscriber already has some tables present? For
example, I did not see the post saying anything like "Only if the
table does not already exist then it will be created".
On the contrary, the post seemed to say SUBREL_STATE_CREATE 'c' would
*always* be set when this subscriber mode is enabled. And then it
seemed to say the table would *always* get re-created by the tablesync
in this new mode.
Won't this cause problems
- if the user wanted a slightly different subscriber-side table? (eg
some extra columns on the subscriber-side table)
- if there was some pre-existing table data on the subscriber-side
table that you now are about to re-create and clobber?
Or does the idea intend that the CREATE TABLE DDL that will be
executed is like "CREATE TABLE ... IF NOT EXISTS"?
~~~
Q2.
The post says. "DDL replication is required for this feature". And "It
should also have turned on publication of DDL commands."
It wasn't entirely clear to me why those must be a requirement. Is
that just to make implementation easier?
Sure, I see that the idea might have some (or maybe a lot?) of common
internal code with the table DDL replication work, but OTOH an
auto-create feature for subscriber tables seems like it might be a
useful feature to have regardless of the value of the publication
'ddl' parameter.
------
Kind Regards,
Peter Smith.
Fujitsu Australia.
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Initial Schema Sync for Logical Replication
@ 2023-03-18 12:06 Alvaro Herrera <[email protected]>
parent: Kumar, Sachin <[email protected]>
2 siblings, 0 replies; 5+ messages in thread
From: Alvaro Herrera @ 2023-03-18 12:06 UTC (permalink / raw)
To: Kumar, Sachin <[email protected]>; +Cc: [email protected] <[email protected]>
On 2023-Mar-15, Kumar, Sachin wrote:
> 1. In CreateSubscription() when we create replication slot(walrcv_create_slot()), should
> use CRS_EXPORT_SNAPSHOT, So that we can use this snapshot later in the pg_dump.
>
> 2. Now we can call pg_dump with above snapshot from CreateSubscription.
Overall I'm not on board with the idea that logical replication would
depend on pg_dump; that seems like it could run into all sorts of
trouble (what if calling external binaries requires additional security
setup? what about pg_hba connection requirements? what about
max_connections in tight circumstances?).
It would be much better, I think, to handle this internally in the
publisher instead: similar to how DDL sync would work, except it'd
somehow generate the CREATE statements from the existing tables instead
of waiting for DDL events to occur. I grant that this does require
writing a bunch of new code for each object type, a lot of which would
duplicate the pg_dump logic, but it would probably be a lot more robust.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Initial Schema Sync for Logical Replication
@ 2023-03-23 11:15 Amit Kapila <[email protected]>
parent: Kumar, Sachin <[email protected]>
2 siblings, 0 replies; 5+ messages in thread
From: Amit Kapila @ 2023-03-23 11:15 UTC (permalink / raw)
To: Kumar, Sachin <[email protected]>; +Cc: [email protected] <[email protected]>
On Wed, Mar 15, 2023 at 11:12 PM Kumar, Sachin <[email protected]> wrote:
>
>
> Concurrent DDL :-
>
> User can execute a DDL command to table t1 at the same time when subscriber is trying to sync
>
> it. pictorial representation https://imgur.com/a/ivrIEv8 [1]
>
>
>
> In tablesync process, it makes a connection to the publisher and it sees the
>
> table state which can be in future wrt to the publisher, which can introduce conflicts.
>
> For example:-
>
>
>
> CASE 1:- { Publisher removed the column b from the table t1 when subscriber was doing pg_restore
>
> (or any point in concurrent DDL window described in picture [1] ), when tableSync
>
> process will start transaction on the publisher it will see request data of table t1
>
> including column b, which does not exist on the publisher.} So that is why tableSync process
>
> asks for the latest definition.
>
> If we say that we will delay tableSync worker till all the DDL related to table t1 is
>
> applied by the applier process , we can still have a window when publisher issues a DDL
>
> command just before tableSync starts its transaction, and therefore making tableSync and
>
> publisher table definition incompatible (Thanks to Masahiko for pointing out this race
>
> condition).
>
IIUC, this is possible only if tablesync process uses a snapshot
different than the snapshot we have used to perform the initial schema
sync, otherwise, this shouldn't be a problem. Let me try to explain my
understanding with an example (the LSNs used are just explain the
problem):
1. Create Table t1(c1, c2); --LSN: 90
2. Insert t1 (1, 1); --LSN 100
3. Insert t1 (2, 2); --LSN 110
4. Alter t1 Add Column c3; --LSN 120
5. Insert t1 (3, 3, 3); --LSN 130
Now, say before starting tablesync worker, apply process performs
initial schema sync and uses a snapshot corresponding to LSN 100. Then
it starts tablesync process to allow the initial copy of data in t1.
Here, if the table sync process tries to establish a new snapshot, it
may get data till LSN 130 and when it will try to copy the same in
subscriber it will fail. Is my understanding correct about the problem
you described? If so, can't we allow tablesync process to use the same
exported snapshot as we used for the initial schema sync and won't
that solve the problem you described?
>
>
> Applier process will skip all DDL/DMLs related to the table t1 and tableSync will apply those
>
> in Catchup phase.
>
> Although there is one issue what will happen to views/ or functions which depend on the table
>
> . I think they should wait till table_state is > SUBREL_STATE_CREATE (means we have the latest
>
> schema definition from the publisher).
>
> There might be corner cases to this approach or maybe a better way to handle concurrent DDL
>
> One simple solution might be to disallow DDLs on the publisher till all the schema is
>
> synced and all tables have state >= SUBREL_STATE_DATASYNC (We can have CASE 1: issue ,
>
> even with DDL replication, so we have to wait till all the tables have table_state
>
> > SUBREL_STATE_DATASYNC). Which might be a big window for big databases.
>
>
>
>
>
> Refresh publication :-
>
> In refresh publication, subscriber does create a new replication slot hence , we can’t run
>
> pg_dump with a snapshot which starts from origin(maybe this is not an issue at all). In this case
>
> it makes more sense for tableSync worker to do schema sync.
>
Can you please explain this problem with some examples?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-03-23 11:15 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-13 08:12 [PATCH v4] pageinspect: Change block number arguments to bigint Peter Eisentraut <[email protected]>
2023-03-15 17:42 Initial Schema Sync for Logical Replication Kumar, Sachin <[email protected]>
2023-03-16 04:18 ` Re: Initial Schema Sync for Logical Replication Peter Smith <[email protected]>
2023-03-18 12:06 ` Re: Initial Schema Sync for Logical Replication Alvaro Herrera <[email protected]>
2023-03-23 11:15 ` Re: Initial Schema Sync for Logical Replication Amit Kapila <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox