On Fri, 16 Oct 2020 at 19:26, Alvaro Herrera <alvhe...@alvh.no-ip.org>
wrote:
> On 2020-Sep-23, Ashutosh Bapat wrote:
>
> > > You're ignoring the xid use-case, for which DatumGetUInt32 actually is
> > > the right thing.
> >
> > There is DatumGetTransactionId() which should be used instead.
> > That made me search if there's PG_GETARG_TRANSACTIONID() and yes it's
> > there but only defined in xid.c. So pg_xact_commit_timestamp(),
> > pg_xact_commit_timestamp_origin() and pg_get_multixact_members() use
> > PG_GETARG_UNIT32. IMO those should be changed to use
> > PG_GETARG_TRANSACTIONID. That would require moving
> > PG_GETARG_TRANSACTIONID somewhere outside xid.c; may be fmgr.h where
> > other PG_GETARG_* are.
>
> Hmm, yeah, I think this would be a good idea.
>
The patch 0003 does that.
>
> > get_raw_page() also does similar thing but the effect is not as dangerous
> > SELECT octet_length(get_raw_page('test1', 'main', -1)) AS main_1;
> > ERROR: block number 4294967295 is out of range for relation "test1"
> > Similarly for bt_page_stats() and bt_page_items()
>
> Hmm, but page numbers above signed INT_MAX are valid. So this would
> prevent reading all legitimate pages past that.
>
>
According to https://www.postgresql.org/docs/12/datatype-numeric.html,
these functions shouldn't be accepting values higher than INT_MAX since
it's outside the integer data type range. But may be it's a convenient way
to avoid using bigint. Anyway those changes are separate in 0002 patch
which can be discarded as a whole. But for now I am keeping it in the bunch.
--
Best Wishes,
Ashutosh
From c81e432aed62b04fcd7fb6f60f96c76a6a946e4a Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@2ndquadrant.com>
Date: Thu, 22 Oct 2020 15:46:14 +0530
Subject: [PATCH 3/3] Extern'alize PG_GETARG_TRANSACTIONID and
PG_RETURN_TRANSACTIONID
We use PG_GETARG_UINT32 to extract transaction id from an input argument
in UDFs. Instead extern'alize PG_GETARG_TRANSACTIONID defined in xid.c
and use it.
PG_RETURN_TRANSACTIONID has no users outside xid.c. But extern'alized
that too to be symmetric with PG_GETARG_TRANSACTIONID and in case a need
arised in future.
Ashutosh Bapat
---
src/backend/access/transam/commit_ts.c | 4 ++--
src/backend/access/transam/multixact.c | 2 +-
src/backend/utils/adt/xid.c | 2 --
src/include/fmgr.h | 2 ++
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index cb8a968801..2fe551f17e 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -404,7 +404,7 @@ error_commit_ts_disabled(void)
Datum
pg_xact_commit_timestamp(PG_FUNCTION_ARGS)
{
- TransactionId xid = PG_GETARG_UINT32(0);
+ TransactionId xid = PG_GETARG_TRANSACTIONID(0);
TimestampTz ts;
bool found;
@@ -481,7 +481,7 @@ pg_last_committed_xact(PG_FUNCTION_ARGS)
Datum
pg_xact_commit_timestamp_origin(PG_FUNCTION_ARGS)
{
- TransactionId xid = PG_GETARG_UINT32(0);
+ TransactionId xid = PG_GETARG_TRANSACTIONID(0);
RepOriginId nodeid;
TimestampTz ts;
Datum values[2];
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 6ccdc5b58c..4338c664fb 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -3320,7 +3320,7 @@ pg_get_multixact_members(PG_FUNCTION_ARGS)
int nmembers;
int iter;
} mxact;
- MultiXactId mxid = PG_GETARG_UINT32(0);
+ MultiXactId mxid = PG_GETARG_TRANSACTIONID(0);
mxact *multi;
FuncCallContext *funccxt;
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 20389aff1d..0bccfa4755 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -23,8 +23,6 @@
#include "utils/builtins.h"
#include "utils/xid8.h"
-#define PG_GETARG_TRANSACTIONID(n) DatumGetTransactionId(PG_GETARG_DATUM(n))
-#define PG_RETURN_TRANSACTIONID(x) return TransactionIdGetDatum(x)
#define PG_GETARG_COMMANDID(n) DatumGetCommandId(PG_GETARG_DATUM(n))
#define PG_RETURN_COMMANDID(x) return CommandIdGetDatum(x)
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index f25068fae2..ce37e342cd 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -276,6 +276,7 @@ extern struct varlena *pg_detoast_datum_packed(struct varlena *datum);
#define PG_GETARG_POINTER(n) DatumGetPointer(PG_GETARG_DATUM(n))
#define PG_GETARG_CSTRING(n) DatumGetCString(PG_GETARG_DATUM(n))
#define PG_GETARG_NAME(n) DatumGetName(PG_GETARG_DATUM(n))
+#define PG_GETARG_TRANSACTIONID(n) DatumGetTransactionId(PG_GETARG_DATUM(n))
/* these macros hide the pass-by-reference-ness of the datatype: */
#define PG_GETARG_FLOAT4(n) DatumGetFloat4(PG_GETARG_DATUM(n))
#define PG_GETARG_FLOAT8(n) DatumGetFloat8(PG_GETARG_DATUM(n))
@@ -360,6 +361,7 @@ extern struct varlena *pg_detoast_datum_packed(struct varlena *datum);
#define PG_RETURN_POINTER(x) return PointerGetDatum(x)
#define PG_RETURN_CSTRING(x) return CStringGetDatum(x)
#define PG_RETURN_NAME(x) return NameGetDatum(x)
+#define PG_RETURN_TRANSACTIONID(x) return TransactionIdGetDatum(x)
/* these macros hide the pass-by-reference-ness of the datatype: */
#define PG_RETURN_FLOAT4(x) return Float4GetDatum(x)
#define PG_RETURN_FLOAT8(x) return Float8GetDatum(x)
--
2.17.1
From c4ca8cf28125c86ff11a1bd8758aaae2b31344f3 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@2ndquadrant.com>
Date: Tue, 22 Sep 2020 19:00:56 +0530
Subject: [PATCH 1/3] Handle negative number of tuples passed to normal_rand()
The function converts the first argument i.e. the number of tuples to
return into an unsigned integer which turns out to be huge number when a
negative value is passed. This causes the function to take much longer
time to execute. Instead return no rows in that case.
While at it, improve SQL test to test the number of tuples returned by
this function.
Ashutosh Bapat
---
contrib/tablefunc/expected/tablefunc.out | 15 +++++++++++----
contrib/tablefunc/sql/tablefunc.sql | 4 +++-
contrib/tablefunc/tablefunc.c | 4 +++-
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/contrib/tablefunc/expected/tablefunc.out b/contrib/tablefunc/expected/tablefunc.out
index fffadc6e1b..1c9ecef52c 100644
--- a/contrib/tablefunc/expected/tablefunc.out
+++ b/contrib/tablefunc/expected/tablefunc.out
@@ -3,10 +3,17 @@ CREATE EXTENSION tablefunc;
-- normal_rand()
-- no easy way to do this for regression testing
--
-SELECT avg(normal_rand)::int FROM normal_rand(100, 250, 0.2);
- avg
------
- 250
+SELECT avg(normal_rand)::int, count(*) FROM normal_rand(100, 250, 0.2);
+ avg | count
+-----+-------
+ 250 | 100
+(1 row)
+
+-- negative number of tuples
+SELECT avg(normal_rand)::int, count(*) FROM normal_rand(-1, 250, 0.2);
+ avg | count
+-----+-------
+ | 0
(1 row)
--
diff --git a/contrib/tablefunc/sql/tablefunc.sql b/contrib/tablefunc/sql/tablefunc.sql
index ec375b05c6..02e8a98c73 100644
--- a/contrib/tablefunc/sql/tablefunc.sql
+++ b/contrib/tablefunc/sql/tablefunc.sql
@@ -4,7 +4,9 @@ CREATE EXTENSION tablefunc;
-- normal_rand()
-- no easy way to do this for regression testing
--
-SELECT avg(normal_rand)::int FROM normal_rand(100, 250, 0.2);
+SELECT avg(normal_rand)::int, count(*) FROM normal_rand(100, 250, 0.2);
+-- negative number of tuples
+SELECT avg(normal_rand)::int, count(*) FROM normal_rand(-1, 250, 0.2);
--
-- crosstab()
diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c
index 02f02eab57..fd327b1c41 100644
--- a/contrib/tablefunc/tablefunc.c
+++ b/contrib/tablefunc/tablefunc.c
@@ -174,6 +174,7 @@ normal_rand(PG_FUNCTION_ARGS)
FuncCallContext *funcctx;
uint64 call_cntr;
uint64 max_calls;
+ int32 num_tuples;
normal_rand_fctx *fctx;
float8 mean;
float8 stddev;
@@ -193,7 +194,8 @@ normal_rand(PG_FUNCTION_ARGS)
oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
/* total number of tuples to be returned */
- funcctx->max_calls = PG_GETARG_UINT32(0);
+ num_tuples = PG_GETARG_INT32(0);
+ funcctx->max_calls = num_tuples > 0 ? num_tuples : 0;
/* allocate memory for user context */
fctx = (normal_rand_fctx *) palloc(sizeof(normal_rand_fctx));
--
2.17.1
From 40ebfac6e22b3ae282c2bd8e7237d035666c3edf Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@2ndquadrant.com>
Date: Wed, 23 Sep 2020 10:49:10 +0530
Subject: [PATCH 2/3] Negative block number passed to functions in pageinspect
module
When a negative integer is passed to get_raw_page(), bt_page_stats() and
bt_page_inspect() they report an invalid block number error with a bogus
number. This is because the input is casted to an unsigned integer which
for negative numbers happens to be a large value. This, at the worst, is
confusing. Accept the intput as a signed integer and report an error
with negative input as is.
Ashutosh Bapat.
---
contrib/pageinspect/btreefuncs.c | 16 ++++++++++++++--
contrib/pageinspect/expected/btree.out | 4 ++++
contrib/pageinspect/expected/page.out | 4 ++++
contrib/pageinspect/rawpage.c | 18 +++++++++++++++---
contrib/pageinspect/sql/btree.sql | 2 ++
contrib/pageinspect/sql/page.sql | 2 ++
6 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index 445605db58..494c711801 100644
--- a/contrib/pageinspect/btreefuncs.c
+++ b/contrib/pageinspect/btreefuncs.c
@@ -164,7 +164,7 @@ Datum
bt_page_stats(PG_FUNCTION_ARGS)
{
text *relname = PG_GETARG_TEXT_PP(0);
- uint32 blkno = PG_GETARG_UINT32(1);
+ int32 blkno = PG_GETARG_INT32(1);
Buffer buffer;
Relation rel;
RangeVar *relrv;
@@ -200,6 +200,12 @@ bt_page_stats(PG_FUNCTION_ARGS)
if (blkno == 0)
elog(ERROR, "block 0 is a meta page");
+ if (blkno < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number %d",
+ blkno)));
+
CHECK_RELATION_BLOCK_RANGE(rel, blkno);
buffer = ReadBuffer(rel, blkno);
@@ -409,7 +415,7 @@ Datum
bt_page_items(PG_FUNCTION_ARGS)
{
text *relname = PG_GETARG_TEXT_PP(0);
- uint32 blkno = PG_GETARG_UINT32(1);
+ int32 blkno = PG_GETARG_INT32(1);
Datum result;
FuncCallContext *fctx;
MemoryContext mctx;
@@ -420,6 +426,12 @@ bt_page_items(PG_FUNCTION_ARGS)
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to use pageinspect functions")));
+ if (blkno < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number %d",
+ blkno)));
+
if (SRF_IS_FIRSTCALL())
{
RangeVar *relrv;
diff --git a/contrib/pageinspect/expected/btree.out b/contrib/pageinspect/expected/btree.out
index 17bf0c5470..1ee0a6d791 100644
--- a/contrib/pageinspect/expected/btree.out
+++ b/contrib/pageinspect/expected/btree.out
@@ -32,6 +32,8 @@ btpo_flags | 3
SELECT * FROM bt_page_stats('test1_a_idx', 2);
ERROR: block number out of range
+SELECT * FROM bt_page_stats('test1_a_idx', -1);
+ERROR: invalid block number -1
SELECT * FROM bt_page_items('test1_a_idx', 0);
ERROR: block 0 is a meta page
SELECT * FROM bt_page_items('test1_a_idx', 1);
@@ -48,6 +50,8 @@ tids |
SELECT * FROM bt_page_items('test1_a_idx', 2);
ERROR: block number out of range
+SELECT * FROM bt_page_items('test1_a_idx', -1);
+ERROR: invalid block number -1
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 0));
ERROR: block is a meta page
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 1));
diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index b6aea0124b..66de360eed 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -12,6 +12,8 @@ SELECT octet_length(get_raw_page('test1', 'main', 0)) AS main_0;
SELECT octet_length(get_raw_page('test1', 'main', 1)) AS main_1;
ERROR: block number 1 is out of range for relation "test1"
+SELECT octet_length(get_raw_page('test1', 'main', -1)) AS main_1;
+ERROR: invalid block number -1
SELECT octet_length(get_raw_page('test1', 'fsm', 0)) AS fsm_0;
fsm_0
-------
@@ -49,6 +51,8 @@ SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
8192 | 4
(1 row)
+SELECT pagesize, version FROM page_header(get_raw_page('test1', -1));
+ERROR: invalid block number -1
SELECT page_checksum(get_raw_page('test1', 0), 0) IS NOT NULL AS silly_checksum_test;
silly_checksum_test
---------------------
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index c0181506a5..9035a50342 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -46,7 +46,7 @@ Datum
get_raw_page(PG_FUNCTION_ARGS)
{
text *relname = PG_GETARG_TEXT_PP(0);
- uint32 blkno = PG_GETARG_UINT32(1);
+ int32 blkno = PG_GETARG_INT32(1);
bytea *raw_page;
/*
@@ -59,7 +59,13 @@ get_raw_page(PG_FUNCTION_ARGS)
(errmsg("wrong number of arguments to get_raw_page()"),
errhint("Run the updated pageinspect.sql script.")));
- raw_page = get_raw_page_internal(relname, MAIN_FORKNUM, blkno);
+ if (blkno < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number %d",
+ blkno)));
+
+ raw_page = get_raw_page_internal(relname, MAIN_FORKNUM, (uint32) blkno);
PG_RETURN_BYTEA_P(raw_page);
}
@@ -76,10 +82,16 @@ get_raw_page_fork(PG_FUNCTION_ARGS)
{
text *relname = PG_GETARG_TEXT_PP(0);
text *forkname = PG_GETARG_TEXT_PP(1);
- uint32 blkno = PG_GETARG_UINT32(2);
+ int32 blkno = PG_GETARG_INT32(2);
bytea *raw_page;
ForkNumber forknum;
+ if (blkno < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid block number %d",
+ blkno)));
+
forknum = forkname_to_number(text_to_cstring(forkname));
raw_page = get_raw_page_internal(relname, forknum, blkno);
diff --git a/contrib/pageinspect/sql/btree.sql b/contrib/pageinspect/sql/btree.sql
index 8eac64c7b3..ef581f4838 100644
--- a/contrib/pageinspect/sql/btree.sql
+++ b/contrib/pageinspect/sql/btree.sql
@@ -9,10 +9,12 @@ SELECT * FROM bt_metap('test1_a_idx');
SELECT * FROM bt_page_stats('test1_a_idx', 0);
SELECT * FROM bt_page_stats('test1_a_idx', 1);
SELECT * FROM bt_page_stats('test1_a_idx', 2);
+SELECT * FROM bt_page_stats('test1_a_idx', -1);
SELECT * FROM bt_page_items('test1_a_idx', 0);
SELECT * FROM bt_page_items('test1_a_idx', 1);
SELECT * FROM bt_page_items('test1_a_idx', 2);
+SELECT * FROM bt_page_items('test1_a_idx', -1);
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 0));
SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 1));
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index bd049aeb24..05170e34fd 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -10,6 +10,7 @@ VACUUM test1; -- set up FSM
SELECT octet_length(get_raw_page('test1', 'main', 0)) AS main_0;
SELECT octet_length(get_raw_page('test1', 'main', 1)) AS main_1;
+SELECT octet_length(get_raw_page('test1', 'main', -1)) AS main_1;
SELECT octet_length(get_raw_page('test1', 'fsm', 0)) AS fsm_0;
SELECT octet_length(get_raw_page('test1', 'fsm', 1)) AS fsm_1;
@@ -23,6 +24,7 @@ SELECT octet_length(get_raw_page('test1', 'xxx', 0));
SELECT get_raw_page('test1', 0) = get_raw_page('test1', 'main', 0);
SELECT pagesize, version FROM page_header(get_raw_page('test1', 0));
+SELECT pagesize, version FROM page_header(get_raw_page('test1', -1));
SELECT page_checksum(get_raw_page('test1', 0), 0) IS NOT NULL AS silly_checksum_test;
--
2.17.1
