Re: remaining sql/json patches

[Himanshu Upadhyaya]

On Mon, Mar 18, 2024 at 3:33 PM Amit Langote 
wrote:

> Himanshu,
>
> On Mon, Mar 18, 2024 at 4:57 PM Himanshu Upadhyaya
>  wrote:
> > I have tested a nested case  but  why is the negative number allowed in
> subscript(NESTED '$.phones[-1]'COLUMNS), it should error out if the number
> is negative.
> >
> > ‘postgres[170683]=#’SELECT * FROM JSON_TABLE(jsonb '{
> > ‘...>’ "id" : "0.234567897890",
> > ‘...>’ "name" : {
> "first":"John",
> "last":"Doe" },
> > ‘...>’ "phones" : [{"type":"home", "number":"555-3762"},
> > ‘...>’ {"type":"work", "number":"555-7252",
> "test":123}]}',
> > ‘...>’'$'
> > ‘...>’COLUMNS(
> > ‘...>’ id numeric(2,2) PATH 'lax $.id',
> > ‘...>’ last_name varCHAR(10) PATH 'lax $.name.last',
> first_name VARCHAR(10) PATH 'lax $.name.first',
> > ‘...>’  NESTED '$.phones[-1]'COLUMNS (
> > ‘...>’"type" VARCHAR(10),
> > ‘...>’"number" VARCHAR(10)
> > ‘...>’ )
> > ‘...>’  )
> > ‘...>’   ) as t;
> >   id  | last_name | first_name | type | number
> > --+---++--+
> >  0.23 | Doe   | Johnnn |  |
> > (1 row)
>
> You're not getting an error because the default mode of handling
> structural errors in SQL/JSON path expressions is "lax".  If you say
> "strict" in the path string, you will get an error:
>
>
ok, got it, thanks.


> SELECT * FROM JSON_TABLE(jsonb '{
>  "id" : "0.234567897890",
>  "name" : {
> "first":"John",
> "last":"Doe" },
>  "phones" : [{"type":"home", "number":"555-3762"},
>  {"type":"work", "number":"555-7252", "test":123}]}',
> '$'
> COLUMNS(
>  id numeric(2,2) PATH 'lax $.id',
>  last_name varCHAR(10) PATH 'lax $.name.last',
> first_name VARCHAR(10) PATH 'lax $.name.first',
>   NESTED 'strict $.phones[-1]'COLUMNS (
> "type" VARCHAR(10),
> "number" VARCHAR(10)
>  )
>   ) error on error
>) as t;
> ERROR:  jsonpath array subscript is out of bounds
>
> --
> Thanks, Amit Langote
>


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: remaining sql/json patches

[Himanshu Upadhyaya]

I have tested a nested case  but  why is the negative number allowed in
subscript(NESTED '$.phones[-1]'COLUMNS), it should error out if the number
is negative.

‘postgres[170683]=#’SELECT * FROM JSON_TABLE(jsonb '{
‘...>’ "id" : "0.234567897890",
‘...>’ "name" : {
"first":"John",
"last":"Doe" },
‘...>’ "phones" : [{"type":"home", "number":"555-3762"},
‘...>’ {"type":"work", "number":"555-7252",
"test":123}]}',
‘...>’'$'
‘...>’COLUMNS(
‘...>’ id numeric(2,2) PATH 'lax $.id',
‘...>’ last_name varCHAR(10) PATH 'lax $.name.last',
first_name VARCHAR(10) PATH 'lax $.name.first',
‘...>’  NESTED '$.phones[-1]'COLUMNS (
‘...>’"type" VARCHAR(10),
‘...>’"number" VARCHAR(10)
‘...>’ )
‘...>’  )
‘...>’   ) as t;
  id  | last_name | first_name | type | number
--+---++--+
 0.23 | Doe   | Johnnn |  |
(1 row)

Thanks,
Himanshu

Re: remaining sql/json patches

[Himanshu Upadhyaya]

On Tue, Mar 12, 2024 at 5:37 PM Amit Langote 
wrote:

>
>
> SELECT JSON_EXISTS(jsonb '{"customer_name": "test", "salary":1000,
> "department_id":1}', '$ ? (@.department_id == $dept_id && @.salary ==
> $sal)' PASSING 1000 AS sal, 1 as dept_id);
>  json_exists
> -
>  t
> (1 row)
>
> Does that make sense?
>
> Yes, got it. Thanks for the clarification.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: remaining sql/json patches

[Himanshu Upadhyaya]

Hi,

wanted to share the below case:

‘postgres[146443]=#’SELECT JSON_EXISTS(jsonb '{"customer_name": "test",
"salary":1000, "department_id":1}', '$.* ? (@== $dept_id && @ == $sal)'
PASSING 1000 AS sal, 1 as dept_id);
 json_exists
-
 f
(1 row)

isn't it supposed to return "true" as json in input is matching with both
the condition dept_id and salary?

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: remaining sql/json patches

[Himanshu Upadhyaya]

On Wed, Mar 6, 2024 at 9:04 PM Tomas Vondra 
wrote:

>
>
> I'm pretty sure this is the correct & expected behavior. The second
> query treats the value as string (because that's what should happen for
> values in double quotes).
>
>  ok, Then why does the below query provide the correct conversion, even if
we enclose that in double quotes?
‘postgres[102531]=#’SELECT * FROM JSON_TABLE(jsonb '{
 "id" : "1234567890",
 "FULL_NAME" : "JOHN DOE"}',
'$'
COLUMNS(
 name varchar(20) PATH 'lax $.FULL_NAME',
 id int PATH 'lax $.id'
  )
   )
;
   name   | id
--+
 JOHN DOE | 1234567890
(1 row)

and for bigger input(string) it will leave as empty as below.
‘postgres[102531]=#’SELECT * FROM JSON_TABLE(jsonb '{
 "id" : "12345678901",
 "FULL_NAME" : "JOHN DOE"}',
'$'
COLUMNS(
 name varchar(20) PATH 'lax $.FULL_NAME',
 id int PATH 'lax $.id'
  )
   )
;
   name   | id
--+
 JOHN DOE |
(1 row)

seems it is not something to do with data enclosed in double quotes but
somehow related with internal casting it to integer and I think in case of
bigger input it is not able to cast it to integer(as defined under COLUMNS
as id int PATH 'lax $.id')

‘postgres[102531]=#’SELECT * FROM JSON_TABLE(jsonb '{
 "id" : "12345678901",
 "FULL_NAME" : "JOHN DOE"}',
'$'
COLUMNS(
 name varchar(20) PATH 'lax $.FULL_NAME',
 id int PATH 'lax $.id'
  )
   )
;
   name   | id
--+
 JOHN DOE |
(1 row)
)

if it is not able to represent it to integer because of bigger input, it
should error out with a similar error message instead of leaving it empty.

Thoughts?

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: remaining sql/json patches

[Himanshu Upadhyaya]

On Tue, Mar 5, 2024 at 6:52 AM Amit Langote  wrote:

Hi,

I am doing some random testing with the latest patch and found one scenario
that I wanted to share.
consider a below case.

‘postgres[102531]=#’SELECT * FROM JSON_TABLE(jsonb '{
 "id" : 12345678901,
 "FULL_NAME" : "JOHN DOE"}',
'$'
COLUMNS(
 name varchar(20) PATH 'lax $.FULL_NAME',
 id int PATH 'lax $.id'
  )
   )
;
ERROR:  22003: integer out of range
LOCATION:  numeric_int4_opt_error, numeric.c:4385
‘postgres[102531]=#’SELECT * FROM JSON_TABLE(jsonb '{
 "id" : "12345678901",
 "FULL_NAME" : "JOHN DOE"}',
'$'
COLUMNS(
 name varchar(20) PATH 'lax $.FULL_NAME',
 id int PATH 'lax $.id'
  )
   )
;
   name   | id
--+
 JOHN DOE |
(1 row)

The first query throws an error that the integer is "out of range" and is
quite expected but in the second case(when the value is enclosed with ") it
is able to process the JSON object but does not return any relevant
error(in fact processes the JSON but returns it with empty data for "id"
field). I think second query should fail with a similar error.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: CHECK Constraint Deferrable

[Himanshu Upadhyaya]

On Tue, Sep 19, 2023 at 4:14 PM Dean Rasheed 
wrote:

> > I think we should be able to defer one constraint like in the case of
> > foreign key constraint
> >
>
> Agreed. It should be possible to have a mix of deferred and immediate
> constraint checks. In the example, the tbl_chk_1 is set deferred, but
> it fails immediately, which is clearly not right.
>
> Fixed in V3 patch.

> I would say that it's reasonable to limit the scope of this patch to
> table constraints only, and leave domain constraints to a possible
> follow-up patch.
>
> Sure, Agree.

> A few other review comments:
>
> 1. The following produces a WARNING (possibly the same issue already
> reported):
>
> CREATE TABLE foo (a int, b int);
> ALTER TABLE foo ADD CONSTRAINT a_check CHECK (a > 0);
> ALTER TABLE foo ADD CONSTRAINT b_check CHECK (b > 0) DEFERRABLE;
>
> WARNING:  unexpected pg_constraint record found for relation "foo"
>
> fixed in V3 patch.

> 2. I think that equalTupleDescs() should compare the new fields, when
> comparing the 2 sets of check constraints.
>
> Fixed in V3 patch.

> 3. The constraint exclusion code in the planner should ignore
> deferrable check constraints (see get_relation_constraints() in
> src/backend/optimizer/util/plancat.c), otherwise it might incorrectly
> exclude a relation on the basis of a constraint that is temporarily
> violated, and return incorrect query results. For example:
>
> CREATE TABLE foo (a int);
> CREATE TABLE foo_c1 () INHERITS (foo);
> CREATE TABLE foo_c2 () INHERITS (foo);
> ALTER TABLE foo_c2 ADD CONSTRAINT cc CHECK (a != 5) INITIALLY DEFERRED;
>
> BEGIN;
> INSERT INTO foo_c2 VALUES (5);
> SET LOCAL constraint_exclusion TO off;
> SELECT * FROM foo WHERE a = 5;
> SET LOCAL constraint_exclusion TO on;
> SELECT * FROM foo WHERE a = 5;
> ROLLBACK;
>
> Fixed in V3 patch.

> 4. The code in MergeWithExistingConstraint() should prevent inherited
> constraints being merged if their deferrable properties don't match
> (as MergeConstraintsIntoExisting() does, since
> constraints_equivalent() tests the deferrable fields). I.e., the
> following should fail to merge the constraints, since they don't
> match:
>
> DROP TABLE IF EXISTS p,c;
>
> CREATE TABLE p (a int, b int);
> ALTER TABLE p ADD CONSTRAINT c1 CHECK (a > 0) DEFERRABLE;
> ALTER TABLE p ADD CONSTRAINT c2 CHECK (b > 0);
>
> CREATE TABLE c () INHERITS (p);
> ALTER TABLE c ADD CONSTRAINT c1 CHECK (a > 0);
> ALTER TABLE c ADD CONSTRAINT c2 CHECK (b > 0) DEFERRABLE;
>
> I.e., that should produce an error, as happens if c is made to inherit
> p *after* the constraints have been added.
>
> Fixed in V3 patch.

> 5. Instead of just adding the new fields to the end of the ConstrCheck
> struct, and to the end of lists of function parameters like
> StoreRelCheck(), and other related code, it would be more logical to
> put them immediately before the valid/invalid entries, to match the
> order of constraint properties in pg_constraint, and functions like
> CreateConstraintEntry().
>
> Fixed in V3 patch.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: CHECK Constraint Deferrable

[Himanshu Upadhyaya]

On Thu, Sep 14, 2023 at 9:57 AM vignesh C  wrote:

> 2) I was not sure, if the error message change was intentional:
> 2a)
> In Head:
> CREATE FOREIGN TABLE t9(a int CHECK(a<>0) DEFERRABLE) SERVER s1;
> ERROR:  misplaced DEFERRABLE clause
> LINE 1: CREATE FOREIGN TABLE t9(a int CHECK(a<>0) DEFERRABLE) SERVER...
>   ^
> postgres=# CREATE FOREIGN TABLE t9(a int CHECK(a<>0) DEFERRABLE) SERVER s1;
> ERROR:  "t9" is a foreign table
> DETAIL:  Foreign tables cannot have constraint triggers.
>
> 2b)
> In Head:
> postgres=# CREATE FOREIGN TABLE t2(a int CHECK(a<>0)) SERVER s1;
> CREATE FOREIGN TABLE
> postgres=# ALTER FOREIGN TABLE t2 ADD CONSTRAINT t2_chk_1 CHECK(a<>1)
> DEFERRABLE;
> ERROR:  CHECK constraints cannot be marked DEFERRABLE
>
> With patch:
> postgres=# ALTER FOREIGN TABLE t8 ADD CONSTRAINT t8_chk_1 CHECK(a<>1)
> DEFERRABLE;
> ERROR:  "t8" is a foreign table
> DETAIL:  Foreign tables cannot have constraint triggers.
>
> We are creating a constraint trigger for DEFERRED check constraint and as
per implementation of FOREIGN table we are restricting to have a constraint
trigger. I need to do more analysis before reaching to any conclusion, I
think we can restrict this gram.y itself.

> 3) Insert check is not deferred to commit:
> This insert check here is deferred to commit:
> postgres=# CREATE TABLE tbl (i int ) partition by range (i);
> CREATE TABLE tbl_1 PARTITION OF tbl FOR VALUES FROM (0) TO (10);
> CREATE TABLE tbl_2 PARTITION OF tbl FOR VALUES FROM (20) TO (30);
> CREATE TABLE
> CREATE TABLE
> CREATE TABLE
> postgres=# ALTER TABLE tbl ADD CONSTRAINT tbl_chk_1 CHECK(i<>1) DEFERRABLE;
> ALTER TABLE
> postgres=# begin;
> BEGIN
> postgres=*# SET CONSTRAINTS tbl_chk_1 DEFERRED;
> SET CONSTRAINTS
> postgres=*# INSERT INTO tbl values (1);
> INSERT 0 1
> postgres=*# commit;
> ERROR:  new row for relation "tbl_1" violates check constraint "tbl_chk_1"
> DETAIL:  Failing row contains (1).
>
> But the check here is not deferred to commit:
> postgres=# CREATE TABLE tbl (i int check(i<>0) DEFERRABLE) partition
> by range (i);
> CREATE TABLE tbl_1 PARTITION OF tbl FOR VALUES FROM (0) TO (10);
> CREATE TABLE tbl_2 PARTITION OF tbl FOR VALUES FROM (20) TO (30);
> CREATE TABLE
> CREATE TABLE
> CREATE TABLE
> postgres=# ALTER TABLE tbl ADD CONSTRAINT tbl_chk_1 CHECK(i<>1) DEFERRABLE;
> ALTER TABLE
> postgres=# begin;
> BEGIN
> postgres=*#  SET CONSTRAINTS tbl_chk_1 DEFERRED;
> SET CONSTRAINTS
> postgres=*# INSERT INTO tbl values (1);
> ERROR:  new row for relation "tbl_1" violates check constraint "tbl_chk_1"
> DETAIL:  Failing row contains (1).
>
> Fixed in V3 patch.

> 4) There is a new warning popping up now:
> CREATE TABLE tbl_new_3 (i int check(i<>0)) partition by range (i);
> CREATE FOREIGN TABLE ftbl_new_3 PARTITION OF tbl_new_3 FOR VALUES FROM
> (40) TO (50) server s1;
> postgres=# ALTER TABLE tbl_new_3 ADD CONSTRAINT tbl_new_3_chk
> CHECK(i<>1) DEFERRABLE;
> WARNING:  unexpected pg_constraint record found for relation "tbl_new_3"
> ERROR:  "ftbl_new_3" is a foreign table
> DETAIL:  Foreign tables cannot have constraint triggers.
>
> Fixed in V3 patch.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: CHECK Constraint Deferrable

[Himanshu Upadhyaya]

On Mon, Oct 2, 2023 at 8:31 PM Himanshu Upadhyaya <
upadhyaya.himan...@gmail.com> wrote:

V3 patch attached.

>

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com


v3-0001-Implementation-of-CHECK-Constraint-to-make-it.patch
Description: Binary data

Re: CHECK Constraint Deferrable

[Himanshu Upadhyaya]

On Tue, Sep 12, 2023 at 2:56 PM vignesh C  wrote:

> On Thu, 7 Sept 2023 at 17:26, Himanshu Upadhyaya
>  wrote:
> >
> > Attached is v2 of the patch, rebased against the latest HEAD.
>
> Thanks for working on this, few comments:
> 1) "CREATE TABLE check_constr_tbl (i int CHECK(i<>0) DEFERRABLE, t
> text)" is crashing in windows, the same was noticed in CFBot too:
> 2023-09-11 08:11:36.585 UTC [58563][client backend]
> [pg_regress/constraints][13/880:0] LOG:  statement: CREATE TABLE
> check_constr_tbl (i int CHECK(i<>0) DEFERRABLE, t text);
> 2023-09-11 08:11:36.586 UTC [58560][client backend]
> [pg_regress/inherit][15/391:0] LOG:  statement: drop table c1;
> ../src/backend/commands/trigger.c:220:26: runtime error: member access
> within null pointer of type 'struct CreateTrigStmt'
> ==58563==Using libbacktrace symbolizer.
>
> The details of CFBot failure can be seen at [1]
>
> I have tried it with my latest patch on windows environment and not
getting any crash with the above statement, will do further analysis if
this patch also has the same issue.

> 2) Alter of check constraint deferrable is not handled, is this
> intentional?
> CREATE TABLE check_constr_tbl (i int CHECK(i<>0) DEFERRABLE, t text);
> postgres=# alter table check_constr_tbl alter constraint
> check_constr_tbl_i_check not deferrable;
> ERROR:  constraint "check_constr_tbl_i_check" of relation
> "check_constr_tbl" is not a foreign key constraint
>
> This is not allowed for any constraint type but FOREIGN key. I am not very
sure about if there is any limitation with this so wanted to take opinion
from other hackers on this.

> 3) Should we handle this scenario for domains too:
> CREATE DOMAIN c1_check AS INT CHECK(VALUE > 10);
> create table test(c1 c1_check);
> alter domain c1_check ADD check (VALUE > 20) DEFERRABLE INITIALLY DEFERRED;
>
> begin;
> -- should this be deffered
> insert into test values(19);
> ERROR:  value for domain c1_check violates check constraint
> "c1_check_check1"
>
> We are planning to have a follow-up patch once this initial patch is
committed.

> 4) There is one warning:
> heap.c: In function ‘StoreRelCheck’:
> heap.c:2178:24: warning: implicit declaration of function
> ‘CreateTrigger’ [-Wimplicit-function-declaration]
>  2178 | (void) CreateTrigger(trigger, NULL,
> RelationGetRelid(rel),
>   |

Fixed in V3 patch.

> ^
>
> 5) This should be added to typedefs.list file:
> +typedef enum checkConstraintRecheck
> +{
> +   CHECK_RECHECK_DISABLED, /* Recheck of CHECK constraint
> is disabled, so
> +*
> DEFERRED CHECK constraint will be
> +*
> considered as non-deferrable check
> +*
> constraint.  */
> +   CHECK_RECHECK_ENABLED,  /* Recheck of CHECK constraint
> is enabled, so
> +*
> CHECK constraint will be validated but
> +*
> error will not be reported for deferred
> +*
> CHECK constraint. */
> +   CHECK_RECHECK_EXISTING  /* Recheck of existing violated
> CHECK
> +*
> constraint, indicates that this is a
> +*
> deferred recheck of a row that was reported
> +* as
> a potential violation of CHECK
> +*
> CONSTRAINT */
> +}  checkConstraintRecheck;
>
> Fixed in V3 patch.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: CHECK Constraint Deferrable

[Himanshu Upadhyaya]

On Thu, Sep 14, 2023 at 9:57 AM vignesh C  wrote:

> 3) Insert check is not deferred to commit:
> This insert check here is deferred to commit:
> postgres=# CREATE TABLE tbl (i int ) partition by range (i);
> CREATE TABLE tbl_1 PARTITION OF tbl FOR VALUES FROM (0) TO (10);
> CREATE TABLE tbl_2 PARTITION OF tbl FOR VALUES FROM (20) TO (30);
> CREATE TABLE
> CREATE TABLE
> CREATE TABLE
> postgres=# ALTER TABLE tbl ADD CONSTRAINT tbl_chk_1 CHECK(i<>1) DEFERRABLE;
> ALTER TABLE
> postgres=# begin;
> BEGIN
> postgres=*# SET CONSTRAINTS tbl_chk_1 DEFERRED;
> SET CONSTRAINTS
> postgres=*# INSERT INTO tbl values (1);
> INSERT 0 1
> postgres=*# commit;
> ERROR:  new row for relation "tbl_1" violates check constraint "tbl_chk_1"
> DETAIL:  Failing row contains (1).
>
> But the check here is not deferred to commit:
> postgres=# CREATE TABLE tbl (i int check(i<>0) DEFERRABLE) partition
> by range (i);
> CREATE TABLE tbl_1 PARTITION OF tbl FOR VALUES FROM (0) TO (10);
> CREATE TABLE tbl_2 PARTITION OF tbl FOR VALUES FROM (20) TO (30);
> CREATE TABLE
> CREATE TABLE
> CREATE TABLE
> postgres=# ALTER TABLE tbl ADD CONSTRAINT tbl_chk_1 CHECK(i<>1) DEFERRABLE;
> ALTER TABLE
> postgres=# begin;
> BEGIN
> postgres=*#  SET CONSTRAINTS tbl_chk_1 DEFERRED;
> SET CONSTRAINTS
> postgres=*# INSERT INTO tbl values (1);
> ERROR:  new row for relation "tbl_1" violates check constraint "tbl_chk_1"
> DETAIL:  Failing row contains (1).
>
> I dont think it's a problem, in the second case there are two DEFERRABLE
CHECK constraints and you are marking one as DEFERRED but other one will be
INITIALLY IMMEDIATE. so we can use "SET CONSTRAINTS ALL DEFERRED;".
‘postgres[1271421]=#’CREATE TABLE tbl (i int check(i<>0) DEFERRABLE)
partition
‘...>’by range (i);
CREATE TABLE
‘postgres[1271421]=#’CREATE TABLE tbl_1 PARTITION OF tbl FOR VALUES FROM
(0) TO (10);
CREATE TABLE
‘postgres[1271421]=#’CREATE TABLE tbl_2 PARTITION OF tbl FOR VALUES FROM
(20) TO (30);
CREATE TABLE
‘postgres[1271421]=#’ALTER TABLE tbl ADD CONSTRAINT tbl_chk_1 CHECK(i<>1)
DEFERRABLE;
ALTER TABLE
‘postgres[1271421]=#’\d tbl
  Partitioned table "public.tbl"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 i  | integer |   |  |
Partition key: RANGE (i)
Check constraints:
"tbl_chk_1" CHECK (i <> 1) DEFERRABLE
"tbl_i_check" CHECK (i <> 0) DEFERRABLE
Number of partitions: 2 (Use \d+ to list them.)
 ‘postgres[1271421]=#’begin;
BEGIN
‘postgres[1271421]=#*’SET CONSTRAINTS ALL DEFERRED;
SET CONSTRAINTS
‘postgres[1271421]=#*’INSERT INTO tbl values (1);
INSERT 0 1
‘postgres[1271421]=#*’commit;
ERROR:  23514: new row for relation "tbl_1" violates check constraint
"tbl_chk_1"
DETAIL:  Failing row contains (1).
SCHEMA NAME:  public
TABLE NAME:  tbl_1
CONSTRAINT NAME:  tbl_chk_1
LOCATION:  ExecConstraints, execMain.c:2077

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: CHECK Constraint Deferrable

[Himanshu Upadhyaya]

Thanks for the review comments.

On Tue, Sep 12, 2023 at 2:56 PM vignesh C  wrote:

> On Thu, 7 Sept 2023 at 17:26, Himanshu Upadhyaya
>  wrote:
> >
> > Attached is v2 of the patch, rebased against the latest HEAD.
>
> Thanks for working on this, few comments:
> 1) "CREATE TABLE check_constr_tbl (i int CHECK(i<>0) DEFERRABLE, t
> text)" is crashing in windows, the same was noticed in CFBot too:
> 2023-09-11 08:11:36.585 UTC [58563][client backend]
> [pg_regress/constraints][13/880:0] LOG:  statement: CREATE TABLE
> check_constr_tbl (i int CHECK(i<>0) DEFERRABLE, t text);
> 2023-09-11 08:11:36.586 UTC [58560][client backend]
> [pg_regress/inherit][15/391:0] LOG:  statement: drop table c1;
> ../src/backend/commands/trigger.c:220:26: runtime error: member access
> within null pointer of type 'struct CreateTrigStmt'
> ==58563==Using libbacktrace symbolizer.
>
> Will Fix this in my next patch.


> The details of CFBot failure can be seen at [1]
>
> 2) Alter of check constraint deferrable is not handled, is this
> intentional?
> CREATE TABLE check_constr_tbl (i int CHECK(i<>0) DEFERRABLE, t text);
> postgres=# alter table check_constr_tbl alter constraint
> check_constr_tbl_i_check not deferrable;
> ERROR:  constraint "check_constr_tbl_i_check" of relation
> "check_constr_tbl" is not a foreign key constraint
>
> ALTER CONSTRAINT is currently only supported  for FOREIGN KEY, it's even
not supported for UNIQUE constraint as below:
‘postgres[1271421]=#’CREATE TABLE unique_constr_tbl (i int unique
DEFERRABLE, t text);
CREATE TABLE
‘postgres[1271421]=#’\d unique_constr_tbl;
 Table "public.unique_constr_tbl"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 i  | integer |   |  |
 t  | text|   |  |
Indexes:
"unique_constr_tbl_i_key" UNIQUE CONSTRAINT, btree (i) DEFERRABLE
‘postgres[1271421]=#’alter table unique_constr_tbl alter constraint
unique_constr_tbl_i_key not deferrable;
ERROR:  42809: constraint "unique_constr_tbl_i_key" of relation
"unique_constr_tbl" is not a foreign key constraint
LOCATION:  ATExecAlterConstraint, tablecmds.c:11183

I still need to understand the design restriction here, please let me know
if anyone is aware of this?
is it because of dependency on Indexes?

3) Should we handle this scenario for domains too:
> CREATE DOMAIN c1_check AS INT CHECK(VALUE > 10);
> create table test(c1 c1_check);
> alter domain c1_check ADD check (VALUE > 20) DEFERRABLE INITIALLY DEFERRED;
>
> begin;
> -- should this be deffered
> insert into test values(19);
> ERROR:  value for domain c1_check violates check constraint
> "c1_check_check1"
>
> Yes, thanks for notifying, I missed this for CREATE DOMAIN, will analyse
and include in next revision.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: CHECK Constraint Deferrable

[Himanshu Upadhyaya]

On Fri, Sep 8, 2023 at 1:23 PM Dilip Kumar  wrote:

> 2.
> - if ((failed = ExecRelCheck(resultRelInfo, slot, estate)) != NULL)
> + if ((failed = ExecRelCheck(resultRelInfo, slot, estate,
> checkConstraint, )) != NULL && !recheckConstraints)
>
>
> Why recheckConstraints need to get as output from ExecRelCheck?  I
> mean whether it will be rechecked or nor is already known by the
> caller and
>
Yes it will be known to the caller but  ExecRelCheck will set this new
parameter only if any one of the constraint is defined as Deferrable (in
create table statement) and there is a potential constraint violation.

> Whether the constraint failed or passed is known based on the return
> value so why do you need to extra parameter here?
>
> Because if normal CHECK constraint(non Deferrable) is violated, no need to
proceed with the insertion and in that case recheckConstraints will hold
"false" but if Deferrable check constraint is violated, we need to
revalidate the constraint at commit time and recheckConstraints will hold
"true".

>
> 5.
> +typedef enum checkConstraintRecheck
> +{
> + CHECK_RECHECK_DISABLED, /* Recheck of CHECK constraint is disabled, so
> + * DEFERRED CHECK constraint will be
> + * considered as non-deferrable check
> + * constraint.  */
> + CHECK_RECHECK_ENABLED, /* Recheck of CHECK constraint is enabled, so
> + * CHECK constraint will be validated but
> + * error will not be reported for deferred
> + * CHECK constraint. */
> + CHECK_RECHECK_EXISTING /* Recheck of existing violated CHECK
> + * constraint, indicates that this is a
> + * deferred recheck of a row that was reported
> + * as a potential violation of CHECK
> + * CONSTRAINT */
> +} checkConstraintRecheck;
>
> I do not like the naming convention here, especially the words
> RECHECK, DISABLE, and ENABLE. And also the name of the enum is a bit
> off.  We can name it more like a unique constraint
> YES, PARTIAL, EXISTING
>
> I can think of alternative ENUM name as "EnforceDeferredCheck" and  member
as “CHECK_DEFERRED_YES”, “CHECK_DEFRRED_NO” and “CHECK_DEFERRED_EXISTING”.

Thoughts?
-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: CHECK Constraint Deferrable

[Himanshu Upadhyaya]

Attached is v2 of the patch, rebased against the latest HEAD.

Thanks,
Himanshu
From cf6057ebeffd026ae075ec43d573eca1164eff5b Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Thu, 7 Sep 2023 13:19:14 +0530
Subject: [PATCH v2] Implementation of "CHECK Constraint" to make it

Deferrable.
---
 src/backend/access/common/tupdesc.c   |   2 +
 src/backend/catalog/heap.c|  50 --
 src/backend/commands/constraint.c | 116 ++
 src/backend/commands/copyfrom.c   |  10 +-
 src/backend/commands/tablecmds.c  |   8 ++
 src/backend/commands/trigger.c|  43 ++--
 src/backend/executor/execMain.c   |  33 +-
 src/backend/executor/execReplication.c|  10 +-
 src/backend/executor/nodeModifyTable.c|  29 +++---
 src/backend/parser/gram.y |   2 +-
 src/backend/parser/parse_utilcmd.c|   9 +-
 src/backend/utils/cache/relcache.c|   2 +
 src/include/access/tupdesc.h  |   2 +
 src/include/catalog/heap.h|   2 +
 src/include/catalog/pg_proc.dat   |   5 +
 src/include/commands/trigger.h|   2 +
 src/include/executor/executor.h   |  42 +++-
 src/test/regress/expected/constraints.out |  98 ++
 src/test/regress/sql/constraints.sql  |  99 ++
 19 files changed, 518 insertions(+), 46 deletions(-)

diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 7c5c390503..098cb27932 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -204,6 +204,8 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
 cpy->check[i].ccbin = pstrdup(constr->check[i].ccbin);
 cpy->check[i].ccvalid = constr->check[i].ccvalid;
 cpy->check[i].ccnoinherit = constr->check[i].ccnoinherit;
+cpy->check[i].ccdeferrable = constr->check[i].ccdeferrable;
+cpy->check[i].ccdeferred = constr->check[i].ccdeferred;
 			}
 		}
 
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index b42711f574..b595a60b42 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -52,17 +52,20 @@
 #include "catalog/pg_statistic.h"
 #include "catalog/pg_subscription_rel.h"
 #include "catalog/pg_tablespace.h"
+#include "catalog/pg_trigger.h"
 #include "catalog/pg_type.h"
 #include "catalog/storage.h"
 #include "commands/tablecmds.h"
 #include "commands/typecmds.h"
 #include "miscadmin.h"
+#include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_collate.h"
 #include "parser/parse_expr.h"
 #include "parser/parse_relation.h"
+#include "parser/parser.h"
 #include "parser/parsetree.h"
 #include "partitioning/partdesc.h"
 #include "pgstat.h"
@@ -102,7 +105,8 @@ static ObjectAddress AddNewRelationType(const char *typeName,
 static void RelationRemoveInheritance(Oid relid);
 static Oid	StoreRelCheck(Relation rel, const char *ccname, Node *expr,
 		  bool is_validated, bool is_local, int inhcount,
-		  bool is_no_inherit, bool is_internal);
+		  bool is_no_inherit, bool is_internal,
+		  bool is_deferrable, bool initdeferred);
 static void StoreConstraints(Relation rel, List *cooked_constraints,
 			 bool is_internal);
 static bool MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
@@ -2050,13 +2054,15 @@ SetAttrMissing(Oid relid, char *attname, char *value)
 static Oid
 StoreRelCheck(Relation rel, const char *ccname, Node *expr,
 			  bool is_validated, bool is_local, int inhcount,
-			  bool is_no_inherit, bool is_internal)
+			  bool is_no_inherit, bool is_internal,
+			  bool is_deferrable, bool initdeferred)
 {
 	char	   *ccbin;
 	List	   *varList;
 	int			keycount;
 	int16	   *attNos;
 	Oid			constrOid;
+	CreateTrigStmt *trigger;
 
 	/*
 	 * Flatten expression to string form for storage.
@@ -2113,8 +2119,10 @@ StoreRelCheck(Relation rel, const char *ccname, Node *expr,
 		CreateConstraintEntry(ccname,	/* Constraint Name */
 			  RelationGetNamespace(rel),	/* namespace */
 			  CONSTRAINT_CHECK, /* Constraint Type */
-			  false,	/* Is Deferrable */
-			  false,	/* Is Deferred */
+			  is_deferrable,	/* Is Check Constraint
+ * deferrable */
+			  initdeferred, /* Is Check Constraint initially
+			 * deferred */
 			  is_validated,
 			  InvalidOid,	/* no parent constraint */
 			  RelationGetRelid(rel),	/* relation */
@@ -2142,6 +2150,36 @@ StoreRelCheck(Relation rel, const char *ccname, Node *expr,
 			  is_no_inherit,	/* connoinherit */
 			  is_internal); /* internally constructed? */
 
+	/*
+	 * I

Re: CHECK Constraint Deferrable

[Himanshu Upadhyaya]

I can think of one scenario, as below

1) any department should have an employee
2)any employee should be assigned to a department
so, the employee table has a FK to the department table, and another check
constraint should be added to the department table to ensure there should
be one/more employees in this department. It's kind of a deadlock
situation, each one depends on the other one. We cant insert a new
department, coz there is no employee. Also, we can't insert new employee
belongs to this new department, coz the department hasn't been and cant be
added. So if we have a check constraint defined as deferrable we can solve
this problem.

‘postgres[685143]=#’CREATE FUNCTION checkEmpPresent(did int) RETURNS int AS
$$ SELECT count(*) from emp where emp.deptno = did $$ IMMUTABLE LANGUAGE
SQL;
CREATE FUNCTION
‘postgres[685143]=#’alter table dept add constraint check_cons check
(checkEmpPresent(deptno) > 0);
ALTER TABLE
‘postgres[685143]=#’\d dept;
Table "public.dept"
  Column  | Type  | Collation | Nullable | Default
--+---+---+--+-
 deptno   | integer   |   | not null |
 deptname | character(20) |   |  |
Indexes:
"dept_pkey" PRIMARY KEY, btree (deptno)
Check constraints:
"check_cons" CHECK (checkemppresent(deptno) > 0)
Referenced by:
TABLE "emp" CONSTRAINT "fk_cons" FOREIGN KEY (deptno) REFERENCES
dept(deptno)

‘postgres[685143]=#’insert into dept values (1, 'finance');
ERROR:  23514: new row for relation "dept" violates check constraint
"check_cons"
DETAIL:  Failing row contains (1, finance ).
SCHEMA NAME:  public
TABLE NAME:  dept
CONSTRAINT NAME:  check_cons
LOCATION:  ExecConstraints, execMain.c:2069
‘postgres[685143]=#’\d emp;
   Table "public.emp"
 Column | Type  | Collation | Nullable | Default
+---+---+--+-
 empno  | integer   |   |  |
 ename  | character(20) |   |  |
 deptno | integer   |   |  |
Foreign-key constraints:
"fk_cons" FOREIGN KEY (deptno) REFERENCES dept(deptno)

‘postgres[685143]=#’insert into emp values (1001, 'test', 1);
ERROR:  23503: insert or update on table "emp" violates foreign key
constraint "fk_cons"
DETAIL:  Key (deptno)=(1) is not present in table "dept".
SCHEMA NAME:  public
TABLE NAME:  emp
CONSTRAINT NAME:  fk_cons
LOCATION:  ri_ReportViolation, ri_triggers.c:2608

I have tried with v1 patch as below;

‘postgres[685143]=#’alter table dept drop constraint check_cons;
ALTER TABLE
‘postgres[685143]=#’alter table dept add constraint check_cons check
(checkEmpPresent(deptno) > 0) DEFERRABLE INITIALLY DEFERRED;
ALTER TABLE
‘postgres[685143]=#’BEGIN;
BEGIN
‘postgres[685143]=#*’insert into dept values (1, 'finance');
INSERT 0 1
‘postgres[685143]=#*’insert into emp values (1001, 'test', 1);
INSERT 0 1
‘postgres[685143]=#*’commit;
COMMIT
‘postgres[685143]=#’select * from dept;
 deptno |   deptname
+--
  1 | finance
(1 row)

‘postgres[685143]=#’select * from emp;
 empno |ename | deptno
---+--+
  1001 | test |  1
(1 row)

Thanks,
Himanshu

On Fri, Jul 7, 2023 at 5:21 PM Dilip Kumar  wrote:

> On Wed, Jul 5, 2023 at 3:08 PM Himanshu Upadhyaya
>  wrote:
> >
> > Hi,
> >
> > Currently, there is no support for CHECK constraint DEFERRABLE in a
> create table statement.
> > SQL standard specifies that CHECK constraint can be defined as
> DEFERRABLE.
>
> I think this is a valid argument that this is part of SQL standard so
> it would be good addition to PostgreSQL.  So +1 for the feature.
>
> But I am wondering whether there are some real-world use cases for
> deferred CHECK/NOT NULL constraints?  I mean like for foreign key
> constraints if there is a cyclic dependency between two tables then
> deferring the constraint is the simplest way to insert without error.
>
> --
> Regards,
> Dilip Kumar
> EnterpriseDB: http://www.enterprisedb.com
>


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

CHECK Constraint Deferrable

[Himanshu Upadhyaya]

Hi,

Currently, there is no support for CHECK constraint DEFERRABLE in a create
table statement.
SQL standard specifies that CHECK constraint can be defined as DEFERRABLE.

The attached patch is having implementation for CHECK constraint Deferrable
as below:

‘postgres[579453]=#’CREATE TABLE t1 (i int CHECK(i<>0) DEFERRABLE, t text);
CREATE TABLE
‘postgres[579453]=#’\d t1
 Table "public.t1"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 i  | integer |   |  |
 t  | text|   |  |
Check constraints:
"t1_i_check" CHECK (i <> 0) DEFERRABLE

Now we can have a deferrable CHECK constraint, and we can defer the
constraint validation:

‘postgres[579453]=#’BEGIN;
BEGIN
‘postgres[579453]=#*’SET CONSTRAINTS t1_i_check DEFERRED;
SET CONSTRAINTS
‘postgres[579453]=#*’INSERT INTO t1 VALUES (0, 'one'); -- should succeed
INSERT 0 1
‘postgres[579453]=#*’UPDATE t1 SET i = 1 WHERE t = 'one';
UPDATE 1
‘postgres[579453]=#*’COMMIT; -- should succeed
COMMIT

Attaching the initial patch, I will improve it with documentation in my
next version of the patch.

thoughts?



-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From 1eb72b14a3a6914e893854508a071ae835d23245 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Wed, 5 Jul 2023 14:54:37 +0530
Subject: [PATCH v1] Implementation of "CHECK Constraint" to make it
 Deferrable.

---
 src/backend/access/common/tupdesc.c   |   2 +
 src/backend/catalog/heap.c|  50 --
 src/backend/commands/constraint.c | 116 ++
 src/backend/commands/copyfrom.c   |  10 +-
 src/backend/commands/tablecmds.c  |   8 ++
 src/backend/commands/trigger.c|  43 ++--
 src/backend/executor/execMain.c   |  33 +-
 src/backend/executor/execReplication.c|  10 +-
 src/backend/executor/nodeModifyTable.c|  29 +++---
 src/backend/parser/gram.y |   2 +-
 src/backend/parser/parse_utilcmd.c|   9 +-
 src/backend/utils/cache/relcache.c|   2 +
 src/include/access/tupdesc.h  |   2 +
 src/include/catalog/heap.h|   2 +
 src/include/catalog/pg_proc.dat   |   5 +
 src/include/commands/trigger.h|   2 +
 src/include/executor/executor.h   |  42 +++-
 src/test/regress/expected/constraints.out |  98 ++
 src/test/regress/sql/constraints.sql  |  99 ++
 19 files changed, 518 insertions(+), 46 deletions(-)

diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 7c5c390503..098cb27932 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -204,6 +204,8 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
 cpy->check[i].ccbin = pstrdup(constr->check[i].ccbin);
 cpy->check[i].ccvalid = constr->check[i].ccvalid;
 cpy->check[i].ccnoinherit = constr->check[i].ccnoinherit;
+cpy->check[i].ccdeferrable = constr->check[i].ccdeferrable;
+cpy->check[i].ccdeferred = constr->check[i].ccdeferred;
 			}
 		}
 
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 2a0d82aedd..8a1e8e266f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -52,17 +52,20 @@
 #include "catalog/pg_statistic.h"
 #include "catalog/pg_subscription_rel.h"
 #include "catalog/pg_tablespace.h"
+#include "catalog/pg_trigger.h"
 #include "catalog/pg_type.h"
 #include "catalog/storage.h"
 #include "commands/tablecmds.h"
 #include "commands/typecmds.h"
 #include "miscadmin.h"
+#include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_collate.h"
 #include "parser/parse_expr.h"
 #include "parser/parse_relation.h"
+#include "parser/parser.h"
 #include "parser/parsetree.h"
 #include "partitioning/partdesc.h"
 #include "pgstat.h"
@@ -102,7 +105,8 @@ static ObjectAddress AddNewRelationType(const char *typeName,
 static void RelationRemoveInheritance(Oid relid);
 static Oid	StoreRelCheck(Relation rel, const char *ccname, Node *expr,
 		  bool is_validated, bool is_local, int inhcount,
-		  bool is_no_inherit, bool is_internal);
+		  bool is_no_inherit, bool is_internal,
+		  bool is_deferrable, bool initdeferred);
 static void StoreConstraints(Relation rel, List *cooked_constraints,
 			 bool is_internal);
 static bool MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
@@ -2063,13 +2067,15 @@ SetAttrMissing(Oid relid, char *attname, char *value)
 static Oid
 StoreRelCheck(Relatio

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Thu, Mar 23, 2023 at 2:15 AM Andres Freund  wrote:

>
> Currently the new verify_heapam() follows ctid chains when XMAX_INVALID is
> set
> and expects to find an item it can dereference - but I don't think that's
> something we can rely on: Afaics HOT pruning can break chains, but doesn't
> reset xmax.
>
> We have below code which I think takes care of xmin and xmax matching and
if they match then only we add them to the predecessor array.
/*
 * If the next line pointer is a redirect, or if
it's a tuple
 * but the XMAX of this tuple doesn't match the
XMIN of the next
 * tuple, then the two aren't part of the same
update chain and
 * there is nothing more to do.
 */
if (ItemIdIsRedirected(next_lp))
continue;
curr_htup = (HeapTupleHeader) PageGetItem(ctx.page,
curr_lp);
curr_xmax = HeapTupleHeaderGetUpdateXid(curr_htup);
next_htup = (HeapTupleHeader) PageGetItem(ctx.page,
next_lp);
next_xmin = HeapTupleHeaderGetXmin(next_htup);
if (!TransactionIdIsValid(curr_xmax) ||
!TransactionIdEquals(curr_xmax, next_xmin))
continue;

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Wed, Mar 8, 2023 at 7:30 PM Himanshu Upadhyaya <
upadhyaya.himan...@gmail.com> wrote:
Please find the v11 patch with all these changes.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From f2b262e95fe07dddfec994f20a6d2e76bc12b410 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Thu, 9 Mar 2023 21:18:58 +0530
Subject: [PATCH v11] Implement HOT chain validation in verify_heapam()

Himanshu Upadhyaya, reviewed by Robert Haas, Aleksander Alekseev, Andres Freund.
Some revisions by Robert Haas.
---
 contrib/amcheck/verify_heapam.c   | 291 +-
 src/bin/pg_amcheck/t/004_verify_heapam.pl | 288 -
 2 files changed, 562 insertions(+), 17 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 4fcfd6df72..9cd4a795a0 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -150,7 +150,9 @@ typedef struct HeapCheckContext
 } HeapCheckContext;
 
 /* Internal implementation */
-static void check_tuple(HeapCheckContext *ctx);
+static void check_tuple(HeapCheckContext *ctx,
+		bool *xmin_commit_status_ok,
+		XidCommitStatus *xmin_commit_status);
 static void check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
 			  ToastedAttribute *ta, int32 *expected_chunk_seq,
 			  uint32 extsize);
@@ -160,7 +162,9 @@ static void check_toasted_attribute(HeapCheckContext *ctx,
 	ToastedAttribute *ta);
 
 static bool check_tuple_header(HeapCheckContext *ctx);
-static bool check_tuple_visibility(HeapCheckContext *ctx);
+static bool check_tuple_visibility(HeapCheckContext *ctx,
+   bool *xmin_commit_status_ok,
+   XidCommitStatus *xmin_commit_status);
 
 static void report_corruption(HeapCheckContext *ctx, char *msg);
 static void report_toast_corruption(HeapCheckContext *ctx,
@@ -399,9 +403,16 @@ verify_heapam(PG_FUNCTION_ARGS)
 	for (ctx.blkno = first_block; ctx.blkno <= last_block; ctx.blkno++)
 	{
 		OffsetNumber maxoff;
+		OffsetNumber predecessor[MaxOffsetNumber];
+		OffsetNumber successor[MaxOffsetNumber];
+		bool		lp_valid[MaxOffsetNumber];
+		bool		xmin_commit_status_ok[MaxOffsetNumber];
+		XidCommitStatus	xmin_commit_status[MaxOffsetNumber];
 
 		CHECK_FOR_INTERRUPTS();
 
+		memset(predecessor, 0, sizeof(OffsetNumber) * MaxOffsetNumber);
+
 		/* Optionally skip over all-frozen or all-visible blocks */
 		if (skip_option != SKIP_PAGES_NONE)
 		{
@@ -433,6 +444,12 @@ verify_heapam(PG_FUNCTION_ARGS)
 		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
 			 ctx.offnum = OffsetNumberNext(ctx.offnum))
 		{
+			BlockNumber	nextblkno;
+			OffsetNumber nextoffnum;
+
+			successor[ctx.offnum] = InvalidOffsetNumber;
+			lp_valid[ctx.offnum] = false;
+			xmin_commit_status_ok[ctx.offnum] = false;
 			ctx.itemid = PageGetItemId(ctx.page, ctx.offnum);
 
 			/* Skip over unused/dead line pointers */
@@ -469,6 +486,14 @@ verify_heapam(PG_FUNCTION_ARGS)
 	report_corruption(,
 	  psprintf("line pointer redirection to unused item at offset %u",
 			   (unsigned) rdoffnum));
+
+/*
+ * Record the fact that this line pointer has passed basic
+ * sanity checking, and also the offset number to which it
+ * points.
+ */
+lp_valid[ctx.offnum] = true;
+successor[ctx.offnum] = rdoffnum;
 continue;
 			}
 
@@ -502,11 +527,237 @@ verify_heapam(PG_FUNCTION_ARGS)
 			}
 
 			/* It should be safe to examine the tuple's header, at least */
+			lp_valid[ctx.offnum] = true;
 			ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page, ctx.itemid);
 			ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
 
 			/* Ok, ready to check this next tuple */
-			check_tuple();
+			check_tuple(,
+		_commit_status_ok[ctx.offnum],
+		_commit_status[ctx.offnum]);
+
+			/*
+			 * If the CTID field of this tuple seems to point to another tuple
+			 * on the same page, record that tuple as the successor of this
+			 * one.
+			 */
+			nextblkno = ItemPointerGetBlockNumber(&(ctx.tuphdr)->t_ctid);
+			nextoffnum = ItemPointerGetOffsetNumber(&(ctx.tuphdr)->t_ctid);
+			if (nextblkno == ctx.blkno && nextoffnum != ctx.offnum)
+successor[ctx.offnum] = nextoffnum;
+		}
+
+		/*
+		 * Update chain validation. Check each line pointer that's got a valid
+		 * successor against that successor.
+		 */
+		ctx.attnum = -1;
+		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
+			 ctx.offnum = OffsetNumberNext(ctx.offnum))
+		{
+			ItemId		curr_lp;
+			ItemId		next_lp;
+			HeapTupleHeader curr_htup;
+			HeapTupleHeader next_htup;
+			TransactionId curr_xmin;
+			TransactionId curr_xmax;
+			TransactionId next_xmin;
+			OffsetNumber nextoffnum = successor[ctx.offnum];
+
+			/*
+			 * The current line pointer may not have a successor, either
+			 * because it's not valid or because it didn't point to anything.
+			 * In either case, we have to give up.
+			 *
+			 * If the cur

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Wed, Mar 8, 2023 at 7:06 PM Robert Haas  wrote:

>
> > +/* HOT chains should not intersect. */
> > +if (predecessor[nextoffnum] != InvalidOffsetNumber)
> > +{
> > +report_corruption(,
> > +  psprintf("redirect line pointer
> > points to offset %u, but offset %u also points there",
> > +   (unsigned) nextoffnum,
> > (unsigned) predecessor[nextoffnum]));
> > +continue;
> > +}
> > ```
> >
> > This type of corruption doesn't seem to be test-covered.
>
> Himanshu, would you be able to try to write a test case for this? I
> think you need something like this: update a tuple with a lower TID to
> produce a tuple with a higher TID, e.g. (0,10) is updated to produce
> (0,11). But then have a redirect line pointer that also points to the
> result of the update, in this case (0,11).
>
> Sure Robert, I will work on this.

> > ```
> > +/*
> > + * If the next line pointer is a redirect, or if it's a
> tuple
> > + * but the XMAX of this tuple doesn't match the XMIN of the
> next
> > + * tuple, then the two aren't part of the same update chain
> and
> > + * there is nothing more to do.
> > + */
> > +if (ItemIdIsRedirected(next_lp))
> > +continue;
> > ```
> >
> > lcov shows that the `continue` path is never executed. This is
> > probably not a big deal however.
>
> It might be good to have a negative test case for this, though. Let's
> say we, e.g. update (0,1) to produce (0,2), but then abort. The page
> is HOT-pruned. Then we add insert a new tuple at (0,2), HOT-update it
> to produce (0,3), and commit. Then we HOT-prune again. Possibly we
> could try to write a test case that verifies that this does NOT
> produce any corruption indication.
>
> will work on this too.

> > ```
> > +$node->append_conf('postgresql.conf','max_prepared_transactions=100');
> > ```
> >
> > From what I can tell this line is not needed.
>
> That surprises me, because the new test cases involve preparing a
> transaction, and by default max_prepared_transactions=0. So it seems
> to me (without testing) that this ought to be required. Did you test
> that it works without this setting?
>
> The value of 100 seems a bit excessive, though. Most TAP tests seem to use
> 10.
>
> We need this for prepare transaction, will change it to 10.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Wed, Feb 8, 2023 at 11:17 PM Robert Haas  wrote:

> On Sun, Feb 5, 2023 at 3:57 AM Himanshu Upadhyaya
>  wrote:
> > Thanks, yes it's working fine with Prepared Transaction.
> > Please find attached the v9 patch incorporating all the review comments.
>
> I don't know quite how we're still going around in circles about this,
> but this code makes no sense to me at all:
>
> /*
>  * Add data to the predecessor array even if the current or
>  * successor's LP is not valid. We will not process/validate
> these
>  * offset entries while looping over the predecessor array but
>  * having all entries in the predecessor array will help in
>  * identifying(and validating) the Root of a chain.
>  */
> if (!lp_valid[ctx.offnum] || !lp_valid[nextoffnum])
> {
> predecessor[nextoffnum] = ctx.offnum;
> continue;
> }
>
> If the current offset number is not for a valid line pointer, then it
> makes no sense to talk about the successor. An invalid redirected line
> pointer is one that points off the end of the line pointer array, or
> to before the beginning of the line pointer array, or to a line
> pointer that is unused. An invalid line pointer that is LP_USED is one
> which points to a location outside the page, or to a location inside
> the page. In none of these cases does it make any sense to talk about
> the next tuple. If the line pointer isn't valid, it's pointing to some
> invalid location where there cannot possibly be a tuple. In other
> words, if lp_valid[ctx.offnum] is false, then nextoffnum is a garbage
> value, and therefore referencing predecessor[nextoffnum] is useless
> and dangerous.
>
> If the next offset number is not for a valid line pointer, we could in
> theory still assign to the predecessor array, as you propose here. In
> that case, the tuple or line pointer at ctx.offnum is pointing to the
> line pointer at nextoffnum and that is all fine. But what is the
> point? The comment claims that the point is that it will help us
> identify and validate the root of the hot chain. But if the line
> pointer at nextoffnum is not valid, it can't be the root of a hot
> chain. When we're talking about the root of a HOT chain, we're
> speaking about a tuple. If lp_valid[nextoffnum] is false, there is no
> tuple. Instead of pointing to a tuple, that line pointer is pointing
> to garbage.
>
>
Initially while implementing logic to identify the root of the HOT chain
I was getting crash and regression failure's that time I thought of having
this check along with a few other changes that were required,
but you are right, it's unnecessary to add data to the predecessor
array(in this case) and is not required. I am removing this from the patch.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From 19c93cce0189150d6bfe68237eb3d5a414a18ad9 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Thu, 9 Feb 2023 22:00:25 +0530
Subject: [PATCH v10] Implement HOT chain validation in verify_heapam()

Himanshu Upadhyaya, reviewed by Robert Haas, Aleksander Alekseev, Andres Freund
---
 contrib/amcheck/verify_heapam.c   | 303 +-
 src/bin/pg_amcheck/t/004_verify_heapam.pl | 241 -
 2 files changed, 527 insertions(+), 17 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 4fcfd6df72..7fc984dd33 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -150,7 +150,7 @@ typedef struct HeapCheckContext
 } HeapCheckContext;
 
 /* Internal implementation */
-static void check_tuple(HeapCheckContext *ctx);
+static void check_tuple(HeapCheckContext *ctx, bool *lp_valid);
 static void check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
 			  ToastedAttribute *ta, int32 *expected_chunk_seq,
 			  uint32 extsize);
@@ -160,7 +160,7 @@ static void check_toasted_attribute(HeapCheckContext *ctx,
 	ToastedAttribute *ta);
 
 static bool check_tuple_header(HeapCheckContext *ctx);
-static bool check_tuple_visibility(HeapCheckContext *ctx);
+static bool check_tuple_visibility(HeapCheckContext *ctx, bool *lp_valid);
 
 static void report_corruption(HeapCheckContext *ctx, char *msg);
 static void report_toast_corruption(HeapCheckContext *ctx,
@@ -399,9 +399,14 @@ verify_heapam(PG_FUNCTION_ARGS)
 	for (ctx.blkno = first_block; ctx.blkno <= last_block; ctx.blkno++)
 	{
 		OffsetNumber maxoff;
+		OffsetNumber predecessor[MaxOffsetNumber];
+		OffsetNumber successor[MaxOffsetNumber];
+		bool		lp_valid[MaxOffsetNumber];
 
 		CHECK_FOR_INTERRUPTS();
 
+		memset(predecessor, 0, sizeof(OffsetNumber) * MaxOffsetNumber);
+
 		/* Optionally skip over all-frozen or all-visible blocks */
 		i

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Tue, Jan 31, 2023 at 7:20 PM Robert Haas  wrote:

> On Mon, Jan 30, 2023 at 8:24 AM Himanshu Upadhyaya
>  wrote:
> > Before this we stop the node by "$node->stop;" and then only we progress
> to
> > manual corruption. This will abort all running/in-progress transactions.
> > So, if we create an in-progress transaction and comment "$node->stop;"
> > then somehow all the code that we have for manual corruption does not
> work.
> >
> > I think it is required to stop the server and then only proceed for
> manual corruption?
> > If this is the case then please suggest if there is a way to get an
> in-progress transaction
> > that we can use for manual corruption.
>
> How about using a prepared transaction?
>
> Thanks, yes it's working fine with Prepared Transaction.
Please find attached the v9 patch incorporating all the review comments.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From d241937841c5990ff4df00d1abc6bfbb3491ac3e Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Sun, 5 Feb 2023 12:50:21 +0530
Subject: [PATCH v9] Implement HOT chain validation in verify_heapam()

Himanshu Upadhyaya, reviewed by Robert Haas, Aleksander Alekseev, Andres Freund
---
 contrib/amcheck/verify_heapam.c   | 297 ++
 src/bin/pg_amcheck/t/004_verify_heapam.pl | 241 +-
 2 files changed, 527 insertions(+), 11 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 4fcfd6df72..01c639b5e1 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -399,9 +399,14 @@ verify_heapam(PG_FUNCTION_ARGS)
 	for (ctx.blkno = first_block; ctx.blkno <= last_block; ctx.blkno++)
 	{
 		OffsetNumber maxoff;
+		OffsetNumber predecessor[MaxOffsetNumber];
+		OffsetNumber successor[MaxOffsetNumber];
+		bool		lp_valid[MaxOffsetNumber];
 
 		CHECK_FOR_INTERRUPTS();
 
+		memset(predecessor, 0, sizeof(OffsetNumber) * MaxOffsetNumber);
+
 		/* Optionally skip over all-frozen or all-visible blocks */
 		if (skip_option != SKIP_PAGES_NONE)
 		{
@@ -433,6 +438,10 @@ verify_heapam(PG_FUNCTION_ARGS)
 		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
 			 ctx.offnum = OffsetNumberNext(ctx.offnum))
 		{
+			OffsetNumber nextoffnum;
+
+			successor[ctx.offnum] = InvalidOffsetNumber;
+			lp_valid[ctx.offnum] = false;
 			ctx.itemid = PageGetItemId(ctx.page, ctx.offnum);
 
 			/* Skip over unused/dead line pointers */
@@ -469,6 +478,13 @@ verify_heapam(PG_FUNCTION_ARGS)
 	report_corruption(,
 	  psprintf("line pointer redirection to unused item at offset %u",
 			   (unsigned) rdoffnum));
+
+/*
+ * Make entry in successor array, redirected lp will be
+ * validated at the time when we loop over successor array.
+ */
+successor[ctx.offnum] = rdoffnum;
+lp_valid[ctx.offnum] = true;
 continue;
 			}
 
@@ -504,9 +520,283 @@ verify_heapam(PG_FUNCTION_ARGS)
 			/* It should be safe to examine the tuple's header, at least */
 			ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page, ctx.itemid);
 			ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
+			lp_valid[ctx.offnum] = true;
 
 			/* Ok, ready to check this next tuple */
 			check_tuple();
+
+			/*
+			 * Add the data to the successor array if next updated tuple is in
+			 * the same page. It will be used later to generate the
+			 * predecessor array.
+			 *
+			 * We need to access the tuple's header to populate the
+			 * predecessor array. However the tuple is not necessarily sanity
+			 * checked yet so delaying construction of predecessor array until
+			 * all tuples are sanity checked.
+			 */
+			nextoffnum = ItemPointerGetOffsetNumber(&(ctx.tuphdr)->t_ctid);
+			if (ItemPointerGetBlockNumber(&(ctx.tuphdr)->t_ctid) == ctx.blkno &&
+nextoffnum != ctx.offnum)
+			{
+successor[ctx.offnum] = nextoffnum;
+			}
+		}
+
+		/*
+		 * Loop over offset and populate predecessor array from all entries
+		 * that are present in successor array.
+		 */
+		ctx.attnum = -1;
+		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
+			 ctx.offnum = OffsetNumberNext(ctx.offnum))
+		{
+			ItemId		curr_lp;
+			ItemId		next_lp;
+			HeapTupleHeader curr_htup;
+			HeapTupleHeader next_htup;
+			TransactionId curr_xmax;
+			TransactionId next_xmin;
+
+			OffsetNumber nextoffnum = successor[ctx.offnum];
+
+			if (nextoffnum == InvalidOffsetNumber)
+			{
+/*
+ * This is either the last updated tuple in the chain or a
+ * corrupted Tuple/lp or unused/dead line pointer.
+ */
+continue;
+			}
+
+			/*
+			 * Add data to the predecessor array even if the current or
+			 * successor's LP is not valid. We will not process/validate these
+			 * offset entries while looping over the predecessor array but
+			 * having all entries in the pre

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

Hi Hackers,

On Sun, Jan 22, 2023 at 8:48 PM Himanshu Upadhyaya <
upadhyaya.himan...@gmail.com> wrote:

>
> The test if (pred_in_progress || TransactionIdDidCommit(curr_xmin))
>> seems wrong to me. Shouldn't it be &&? Has this code been tested at
>> all?  It doesn't seem to have a test case. Some of these other errors
>> don't, either. Maybe there's some that we can't easily test in an
>> automated way, but we should test what we can. I guess maybe casual
>> testing wouldn't reveal the problem here because of the recheck, but
>> it's worrying to find logic that doesn't look right with no
>> corresponding comments or test cases.
>>
>> This is totally my Mistake, apologies for that. I will fix this in my
> next patch. Regarding the missing test cases, I need one in-progress
> transaction for these test cases to be included in 004_verify_heapam.pl
> but I don't find a clear way to have an in-progress transaction(as per the
> design of 004_verify_heapam.pl ) that I can use in the test cases. I will
> be doing more research on a solution to add these missing test cases.
>
>>
>> I am trying to add test cases related to in-progress transactions in
004_verify_heapam.pl but I am not able to find a proper way to achieve
this.
We have a logic where we manually corrupt each tuple.
Please refer to the code just after the below comment in
004_verify_heapam.pl

"# Corrupt the tuples, one type of corruption per tuple.  Some types of
# corruption cause verify_heapam to skip to the next tuple without
# performing any remaining checks, so we can't exercise the system properly
if
# we focus all our corruption on a single tuple."

Before this we stop the node by "$node->stop;" and then only we progress to
manual corruption. This will abort all running/in-progress transactions.
So, if we create an in-progress transaction and comment "$node->stop;"
then somehow all the code that we have for manual corruption does not work.

I think it is required to stop the server and then only proceed for manual
corruption?
If this is the case then please suggest if there is a way to get an
in-progress transaction
that we can use for manual corruption.
-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

m.pl ) that I can use in the test cases. I will be doing
more research on a solution to add these missing test cases.

> Some error message kibitizing:
>
>  psprintf("redirected tuple at line pointer offset %u is not heap only
> tuple",
>
> It seems to me that this should say "redirected line pointer pointing
> to a non-heap-only tuple at offset %u". There is no such thing as a
> redirected tuple -- and even if there were, what we have here is
> clearly a redirected line pointer.
>
> psprintf("redirected tuple at line pointer offset %u is not heap only
> tuple",
>
> And I think for the same reasons this one should say something like
> "redirected line pointer pointing to a non-heap-only tuple at offset
> %u".
>
>  psprintf("redirected tuple at line pointer offset %u is not heap
> updated tuple",
>
> Possibly all of these would sound better with "points" rather than
> "pointing" -- if so, we'd need to change an existing message in the
> same way.
>
> And this one should say something like "redirected line pointer
> pointing to a tuple not produced by an update at offset %u".
>
>  psprintf("tuple is root of chain but it is marked as heap-only tuple"));
>
> I think this would sound better if you deleted the word "it".
>
> Will change accordingly in my next patch.

> I don't know whether it's worth arguing about -- it feels like we've
> argued too much already about this sort of thing -- but I am not very
> convinced by initializers like OffsetNumber
> predecessor[MaxOffsetNumber] = {InvalidOffsetNumber}. That style is
> only correct because InvalidOffsetNumber happens to be zero. If it
> were up to me, I'd use memset to clear the predecessor array. I would
> not bulk initialize sucessor and lp_valid but make sure that the first
> loop always sets them, possibly by having the top of the loop set them
> to InvalidOffsetNumber and false initially and then letting code later
> in the loop change the value, or possibly in some other way.
>
> agree, will fix in my next patch

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Fri, Dec 2, 2022 at 5:43 AM Andres Freund  wrote:

>
> > + /* Loop over offsets and validate the data in the
> predecessor array. */
> > + for (OffsetNumber currentoffnum = FirstOffsetNumber;
> currentoffnum <= maxoff;
> > +  currentoffnum = OffsetNumberNext(currentoffnum))
> > + {
> > + HeapTupleHeader pred_htup;
> > + HeapTupleHeader curr_htup;
> > + TransactionId pred_xmin;
> > + TransactionId curr_xmin;
> > + ItemId  pred_lp;
> > + ItemId  curr_lp;
> > + boolpred_in_progress;
> > + XidCommitStatus xid_commit_status;
> > + XidBoundsViolation xid_status;
> > +
> > + ctx.offnum = predecessor[currentoffnum];
> > + ctx.attnum = -1;
> > + curr_lp = PageGetItemId(ctx.page, currentoffnum);
> > + if (!lp_valid[currentoffnum] ||
> ItemIdIsRedirected(curr_lp))
> > + continue;
>
> I don't think we should do PageGetItemId(ctx.page, currentoffnum); if
> !lp_valid[currentoffnum].
>
> Fixed.

>
> > + if (ctx.offnum == 0)
>
> For one, I think it'd be better to use InvalidOffsetNumber here. But more
> generally, storing the predecessor in ctx.offnum seems quite confusing.
>
> changed all relevant places to use  InvalidOffsetNumber.

>
> > + {
> > + /*
> > +  * No harm in overriding value of
> ctx.offnum as we will always
> > +  * continue if we are here.
> > +  */
> > + ctx.offnum = currentoffnum;
> > + if (TransactionIdIsInProgress(curr_xmin)
> || TransactionIdDidCommit(curr_xmin))
>
> Is it actually ok to call TransactionIdDidCommit() here? There's a reason
> get_xid_status() exists after all. And we do have the xid status for xmin
> already, so this could just check xid_commit_status, no?
>
>
> I think it will be good to pass NULL to get_xid_status like
"get_xid_status(curr_xmin, , NULL);" so that we can only check the xid
status at the time when it is actually required. This way we can avoid
checking xid status in cases when we simply 'continue' due to some check.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From 02f0d19bd82f390df8e3d732d60cf3eb0ae1dc97 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Mon, 5 Dec 2022 18:28:33 +0530
Subject: [PATCH v8] Implement HOT chain validation in verify_heapam()

Himanshu Upadhyaya, reviewed by Robert Haas, Aleksander Alekseev, Andres Freund
---
 contrib/amcheck/verify_heapam.c   | 278 ++
 src/bin/pg_amcheck/t/004_verify_heapam.pl | 193 ++-
 2 files changed, 460 insertions(+), 11 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index b72a5c96d1..ebad73d64b 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -399,6 +399,9 @@ verify_heapam(PG_FUNCTION_ARGS)
 	for (ctx.blkno = first_block; ctx.blkno <= last_block; ctx.blkno++)
 	{
 		OffsetNumber maxoff;
+		OffsetNumber predecessor[MaxOffsetNumber] = {InvalidOffsetNumber};
+		OffsetNumber successor[MaxOffsetNumber] = {InvalidOffsetNumber};
+		bool		lp_valid[MaxOffsetNumber] = {false};
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -433,6 +436,8 @@ verify_heapam(PG_FUNCTION_ARGS)
 		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
 			 ctx.offnum = OffsetNumberNext(ctx.offnum))
 		{
+			OffsetNumber nextoffnum;
+
 			ctx.itemid = PageGetItemId(ctx.page, ctx.offnum);
 
 			/* Skip over unused/dead line pointers */
@@ -469,6 +474,13 @@ verify_heapam(PG_FUNCTION_ARGS)
 	report_corruption(,
 	  psprintf("line pointer redirection to unused item at offset %u",
 			   (unsigned) rdoffnum));
+
+/*
+ * make entry in successor array, redirected tuple will be
+ * validated at the time when we loop over successor array
+ */
+successor[ctx.offnum] = rdoffnum;
+lp_valid[ctx.offnum] = true;
 continue;
 			}
 
@@ -504,9 +516,268 @@ verify_heapam(PG_FUNCTION_ARGS)
 			/* It should be safe to examine the tuple's header, at least */
 			ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page, ctx.itemid);
 			ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
+			lp_valid[ctx.offnum] = true;
 
 			/* Ok, ready to check this next tuple */
 			check_tuple();
+
+			/*
+			 * Add the data to the successor array if next updated

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

Hi,


On Fri, Dec 2, 2022 at 5:43 AM Andres Freund  wrote:

>
> > + curr_htup = (HeapTupleHeader) PageGetItem(ctx.page,
> curr_lp);
> > + curr_xmin = HeapTupleHeaderGetXmin(curr_htup);
> > + xid_status = get_xid_status(curr_xmin, ,
> _commit_status);
> > + if (!(xid_status == XID_BOUNDS_OK || xid_status ==
> XID_INVALID))
> > + continue;
>
> Why can we even get here if the xid status isn't XID_BOUNDS_OK?
>
>

 @@ -504,9 +516,269 @@ verify_heapam(PG_FUNCTION_ARGS)
/* It should be safe to examine the tuple's header,
at least */
ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page,
ctx.itemid);
ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
+   lp_valid[ctx.offnum] = true;

/* Ok, ready to check this next tuple */
check_tuple();

referring above code, check_tuple(); do have this check but we populate
lp_valid before that call.
Populating lp_valid before check_tuple() is intentional because even if we
do changes to get the return status from check_tuple() to populate that in
lp_valid, it will be hard to validate cases that are dependent on aborted
transaction (like "tuple with aborted xmin %u was updated to produce a
tuple at offset %u with committed xmin %u") because check_tuple_visibility
is also looking for aborted xmin and return false if tuple's xmin is
aborted, in fact we can add one more parameter to check_tuple and get the
status of transaction if it is aborted and accordingly set lp_valid to true
but that will add unnecessary complexity and don't find it convincing
implementation. Alternatively, I found rechecking xid_status is simpler and
straight.


>
> > + if (ctx.offnum == 0)
>
> For one, I think it'd be better to use InvalidOffsetNumber here. But more
> generally, storing the predecessor in ctx.offnum seems quite confusing.
>
> ok, I will change it to  InvalidOffsetNumber at all the places, we need
ctx.offnum to have the value of the predecessor array as this will be
internally used by report_corruption function to generate the message(eg.
below), and the format of these message's seems more simple and meaningful
to report corruption.

  report_corruption(,

psprintf("heap-only update produced a non-heap only tuple at offset %u",

   (unsigned) currentoffnum));
Here we don't need to mention ctx.offnum explicitly in the above message as
this will be taken care of by the code below.

"report_corruption_internal(Tuplestorestate *tupstore, TupleDesc tupdesc,
   BlockNumber blkno,
OffsetNumber offnum,
   AttrNumber attnum, char
*msg)
{
Datum   values[HEAPCHECK_RELATION_COLS] = {0};
boolnulls[HEAPCHECK_RELATION_COLS] = {0};
HeapTuple   tuple;

    values[0] = Int64GetDatum(blkno);
values[1] = Int32GetDatum(offnum);"

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

htup = (HeapTupleHeader) PageGetItem(ctx.page,
> curr_lp);
> > + curr_xmax = HeapTupleHeaderGetUpdateXid(curr_htup);
> > + next_lp = PageGetItemId(ctx.page, nextoffnum);
> > + next_htup = (HeapTupleHeader) PageGetItem(ctx.page,
> next_lp);
> > + next_xmin = HeapTupleHeaderGetXmin(next_htup);
> > + if (TransactionIdIsValid(curr_xmax) &&
> > + (TransactionIdEquals(curr_xmax, next_xmin)
> ||
> > +  next_xmin == FrozenTransactionId))
> > + {
> > + if (predecessor[nextoffnum] != 0)
> > + {
> > + report_corruption(,
> > +
>  psprintf("updated version at offset %u is also the updated version of
> tuple at offset %u",
> > +
> (unsigned) nextoffnum, (unsigned) predecessor[nextoffnum]));
> > + continue;
>
> I doubt it is correct to enter this path with next_xmin ==
> FrozenTransactionId. This is following a ctid chain that we normally
> wouldn't
> follow, because it doesn't satisfy the t_self->xmax == t_ctid->xmin
> condition.
>
> removed this frozen check.

> + }
> > +
> > + /* Loop over offsets and validate the data in the
> predecessor array. */
> > + for (OffsetNumber currentoffnum = FirstOffsetNumber;
> currentoffnum <= maxoff;
> > +  currentoffnum = OffsetNumberNext(currentoffnum))
> > + {
> > + HeapTupleHeader pred_htup;
> > + HeapTupleHeader curr_htup;
> > + TransactionId pred_xmin;
> > + TransactionId curr_xmin;
> > + ItemId  pred_lp;
> > + ItemId  curr_lp;
> > +
> > + ctx.offnum = predecessor[currentoffnum];
> > + ctx.attnum = -1;
> > +
> > + if (ctx.offnum == 0)
> > + {
> > + /*
> > +  * Either the root of the chain or an
> xmin-aborted tuple from
> > +  * an abandoned portion of the HOT chain.
> > +  */
>
> Hm - couldn't we check that the tuple could conceivably be at the root of a
> chain? I.e. isn't HEAP_HOT_UPDATED? Or alternatively has an aborted xmin?
>
> Done, I have added code to identify cases of missing offset in the
predecessor[] array and added validation that root of the chain must not be
HEAP_ONLY_TUPLE.

>
> > + continue;
> > + }
> > +
> > + curr_lp = PageGetItemId(ctx.page, currentoffnum);
> > + curr_htup = (HeapTupleHeader) PageGetItem(ctx.page,
> curr_lp);
> > + curr_xmin = HeapTupleHeaderGetXmin(curr_htup);
> > +
> > + ctx.itemid = pred_lp = PageGetItemId(ctx.page,
> ctx.offnum);
> > + pred_htup = (HeapTupleHeader) PageGetItem(ctx.page,
> pred_lp);
> > + pred_xmin = HeapTupleHeaderGetXmin(pred_htup);
> > +
> > +     /*
> > +  * If the predecessor's xmin is aborted or in
> progress, the
> > +  * current tuples xmin should be aborted or in
> progress
> > +  * respectively. Also both xmin's must be equal.
> > +  */
> > + if (!TransactionIdEquals(pred_xmin, curr_xmin) &&
> > + !TransactionIdDidCommit(pred_xmin))
> > + {
> > + report_corruption(,
> > +
>  psprintf("tuple with uncommitted xmin %u was updated to produce a tuple at
> offset %u with differing xmin %u",
> > +
> (unsigned) pred_xmin, (unsigned) currentoffnum, (unsigned)
> curr_xmin));
>
> Is this necessarily true? What about a tuple that was inserted in a
> subtransaction and then updated in another subtransaction of the same
> toplevel
> transaction?
>
>
patch has been updated to handle cases of sub-transaction.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From c8ef516cde2578543b23faba3598ed014a0fb95f Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Wed, 30 Nov 2022 15:43:56 +0530
Subject: [PATCH v7] Implement HOT chain validation in verify_heapam()

Himanshu U

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Tue, Nov 15, 2022 at 3:32 AM Andres Freund  wrote:

>
> > Furthermore, it is
> > possible that successor[x] = successor[x'] since the page might be
> corrupted
> > and we haven't checked otherwise.
> >
> > predecessor[y] = x means that successor[x] = y but in addition we've
> > checked that y is sane, and that x.xmax=y.xmin. If there are multiple
> > tuples for which these conditions hold, we've issued complaints about
> > all but one and entered the last into the predecessor array.
>
> As shown by the isolationtester test I just posted, this doesn't quite work
> right now. Probably fixable.
>
> I don't think we can follow non-HOT ctid chains if they're older than the
> xmin
> horizon, including all cases of xmin being frozen. There's just nothing
> guaranteeing that the tuples are actually "related".
>
> I understand the problem with frozen tuples but don't understand the
concern with non-HOT chains,
could you please help with some explanation around it?


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Wed, Nov 16, 2022 at 12:41 PM Himanshu Upadhyaya <
upadhyaya.himan...@gmail.com> wrote:

>
>
>> > + }
>> > +
>> > + /* Loop over offsets and validate the data in the
>> predecessor array. */
>> > + for (OffsetNumber currentoffnum = FirstOffsetNumber;
>> currentoffnum <= maxoff;
>> > +  currentoffnum = OffsetNumberNext(currentoffnum))
>> > + {
>> > + HeapTupleHeader pred_htup;
>> > + HeapTupleHeader curr_htup;
>> > + TransactionId pred_xmin;
>> > + TransactionId curr_xmin;
>> > + ItemId  pred_lp;
>> > + ItemId  curr_lp;
>> > +
>> > + ctx.offnum = predecessor[currentoffnum];
>> > + ctx.attnum = -1;
>> > +
>> > + if (ctx.offnum == 0)
>> > + {
>> > + /*
>> > +  * Either the root of the chain or an
>> xmin-aborted tuple from
>> > +  * an abandoned portion of the HOT chain.
>> > +  */
>>
>> Hm - couldn't we check that the tuple could conceivably be at the root of
>> a
>> chain? I.e. isn't HEAP_HOT_UPDATED? Or alternatively has an aborted xmin?
>>
>>
>  I don't see a way to check if tuple is at the root of HOT chain because
> predecessor array will always be having either xmin from non-abandoned
> transaction or it will be zero. We can't differentiate root or tuple
> inserted via abandoned transaction.
>
> I was wrong here. I think this can be done and will be doing these changes
in my next patch.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Wed, Nov 16, 2022 at 11:23 PM Robert Haas  wrote:

> On Wed, Nov 16, 2022 at 4:51 AM Himanshu Upadhyaya
>  wrote:
> > yes, got it, have tried to test and it is giving false corruption in
> case of subtransaction.
> > I think a better way to have this check is, we need to check that if
> pred_xmin is
> > aborted then current_xmin should be aborted only. So there is no way
> that we
> > validate corruption with in_progress txid.
>
> Please note that you can't use TransactionIdDidAbort here, because
> that will return false for transactions aborted by a crash. You have
> to check that it's not in progress and then afterwards check that it's
> not committed. Also note that if you check whether it's committed
> first and then check whether it's in progress afterwards, there's a
> race condition: it might commit just after you verify that it isn't
> committed yet, and then it won't be in progress any more and will look
> aborted.
>
> I disagree with the idea that we can't check in progress. I think the
> checks could look something like this:
>
> pred_in_progress = TransactionIdIsInProgress(pred_xmin);
> current_in_progress = TransactionIdIsInProgress(current_xmin);
> if (pred_in_progress)
> {
>  if (current_in_progress)
> return ok;
>  // recheck to avoid race condition
>  if (TransactionIdIsInProgress(pred_xmin))
>  {
>  if (TransactionIdDidCommit(current_xmin))
>  return corruption: predecessor xmin in progress, but
> current xmin committed;
>  else
>  return corruption: predecessor xmin in progress, but
> current xmin aborted;
>  }
>
I think we can have a situation where pred_xmin is in progress but
curr_xmin is aborted, consider below example:
 ‘postgres[14723]=#’BEGIN;
BEGIN
‘postgres[14723]=#*’insert into test2 values (1,1);
INSERT 0 1
‘postgres[14723]=#*’savepoint s1;
SAVEPOINT
‘postgres[14723]=#*’update test2 set a =2;
UPDATE 1
‘postgres[14723]=#*’rollback to savepoint s1;
ROLLBACK

Now pred_xmin is in progress but curr_xmin is aborted, am I missing
anything here?
I think if pred_xmin is aborted and curr_xmin is in progress we should
consider it as a corruption case but vice versa is not true.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Wed, Nov 16, 2022 at 1:58 AM Robert Haas  wrote:

> On Tue, Nov 15, 2022 at 2:50 PM Andres Freund  wrote:
> > On 2022-11-15 11:36:21 -0500, Robert Haas wrote:
> > > On Mon, Nov 14, 2022 at 5:02 PM Andres Freund 
> wrote:
> > > > It seems like we should do a bit more validation within a chain of
> > > > tuples. E.g. that no live tuple can follow an !DidCommit xmin?
> > >
> > > I think this check is already present in stronger form. If we see a
> > > !DidCommit xmin, the xmin of the next tuple in the chain not only
> can't be
> > > committed, but had better be the same.
> >
> > As I think I mentioned before, I don't think the "better be the same"
> aspect
> > is correct, think subxacts. E.g.
> >
> > off 0: xmin: top, xmax: child_1
> > off 1: xmin: child_1, xmax: invalid
> >
> > If top hasn't committed yet, the current logic afaict will warn about
> this
> > situation, no? And I don't think we can generally the subxid parent at
> this
> > point, unfortunately (might have truncated subtrans).
>
> Woops, you're right.


yes, got it, have tried to test and it is giving false corruption in case
of subtransaction.
I think a better way to have this check is, we need to check that if
pred_xmin is
aborted then current_xmin should be aborted only. So there is no way that we
validate corruption with in_progress txid.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

in, curr_xmin) &&
> > + !TransactionIdDidCommit(pred_xmin))
> > + {
> > + report_corruption(,
> > +
>  psprintf("tuple with uncommitted xmin %u was updated to produce a tuple at
> offset %u with differing xmin %u",
> > +
> (unsigned) pred_xmin, (unsigned) currentoffnum, (unsigned)
> curr_xmin));
>
> Is this necessarily true? What about a tuple that was inserted in a
> subtransaction and then updated in another subtransaction of the same
> toplevel
> transaction?
>
>
not sure if I am getting? I have tried with below test and don't see any
issue,

‘postgres[14723]=#’drop table test2;
DROP TABLE
‘postgres[14723]=#’create table test2 (a int, b int primary key);
CREATE TABLE
‘postgres[14723]=#’insert into test2 values (1,1);
INSERT 0 1
‘postgres[14723]=#’BEGIN;
BEGIN
‘postgres[14723]=#*’update test2 set a =2 where a =1;
UPDATE 1
‘postgres[14723]=#*’savepoint s1;
SAVEPOINT
‘postgres[14723]=#*’update test2 set a =6;
UPDATE 1
‘postgres[14723]=#*’rollback to savepoint s1;
ROLLBACK
‘postgres[14723]=#*’update test2 set a =6;
UPDATE 1
‘postgres[14723]=#*’savepoint s2;
SAVEPOINT
‘postgres[14723]=#*’update test2 set a =7;
UPDATE 1
‘postgres[14723]=#*’end;
COMMIT
‘postgres[14723]=#’SELECT lp as tuple, t_xmin, t_xmax, t_field3 as t_cid,
t_ctid,tuple_data_split('test2'::regclass, t_data, t_infomask, t_infomask2,
t_bits), heap_tuple_infomask_flags(t_infomask, t_infomask2) FROM
heap_page_items(get_raw_page('test2', 0));
 tuple | t_xmin | t_xmax | t_cid | t_ctid |   tuple_data_split|
heap_tuple_infomask_flags
---+++---++---+---
 1 |   1254 |   1255 | 0 | (0,2)  | {"\\x0100","\\x0100"} |
("{HEAP_XMIN_COMMITTED,HEAP_HOT_UPDATED}",{})
 2 |   1255 |   1257 | 1 | (0,4)  | {"\\x0200","\\x0100"} |
("{HEAP_COMBOCID,HEAP_UPDATED,HEAP_HOT_UPDATED,HEAP_ONLY_TUPLE}",{})
 3 |   1256 |  0 | 1 | (0,3)  | {"\\x0600","\\x0100"} |
("{HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_UPDATED,HEAP_ONLY_TUPLE}",{})
 4 |   1257 |   1258 | 2 | (0,5)  | {"\\x0600","\\x0100"} |
("{HEAP_COMBOCID,HEAP_UPDATED,HEAP_HOT_UPDATED,HEAP_ONLY_TUPLE}",{})
 5 |   1258 |  0 | 3 | (0,5)  | {"\\x0700","\\x0100"} |
("{HEAP_XMAX_INVALID,HEAP_UPDATED,HEAP_ONLY_TUPLE}",{})
(5 rows)


>
> > + }
> > +
> > + /*
> > +  * If the predecessor's xmin is not frozen, then
> current tuple's
> > +  * shouldn't be either.
> > +  */
> > + if (pred_xmin != FrozenTransactionId && curr_xmin
> == FrozenTransactionId)
> > + {
> > + report_corruption(,
> > +
>  psprintf("unfrozen tuple was updated to produce a tuple at offset %u which
> is frozen",
> > +
> (unsigned) currentoffnum));
> > + }
>
> Can't we have a an update chain that is e.g.
> xmin 10, xmax 5 -> xmin 5, xmax invalid
>
> and a vacuum cutoff of 7? That'd preent the first tuple from being removed,
> but would allow 5 to be frozen.
>
> I think there were recent patches proposing we don't freeze in that case,
> but
> we'll having done that in the past
>
>
Not very sure about this, was trying with such case but found hard to
reproduce this.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Tue, Sep 20, 2022 at 6:43 PM Robert Haas  wrote:

> On Tue, Sep 20, 2022 at 5:00 AM Himanshu Upadhyaya
>  wrote:
> > Please find it attached.
>
> This patch still has no test cases. Just as we have test cases for the
> existing corruption checks, we should have test cases for these new
> corruption checks, showing cases where they actually fire.
>
> Test cases are now part of this v6 patch.


> I think I would be inclined to set lp_valid[x] = true in both the
> redirected and non-redirected case, and then have the very first thing
> that the second loop does be if (nextoffnum == 0 ||
> !lp_valid[ctx.offnum]) continue. I think that would be more clear
> about the intent to ignore line pointers that failed validation. Also,
> if you did it that way, then you could catch the case of a redirected
> line pointer pointing to another redirected line pointer, which is a
> corruption condition that the current code does not appear to check.
>
> Yes, it's a good idea to do this additional validation with a redirected
line pointer. Done.

> +/*
> + * Validation via the predecessor array. 1) If the
> predecessor's
> + * xmin is aborted or in progress, the current tuples xmin
> should
> + * be aborted or in progress respectively. Also both xmin's
> must
> + * be equal. 2) If the predecessor's xmin is not frozen, then
> + * current tuple's shouldn't be either. 3) If the
> predecessor's
> + * xmin is equal to the current tuple's xmin, the current
> tuple's
> + * cmin should be greater than the predecessor's cmin. 4) If
> the
> + * current tuple is not HOT then its predecessor's tuple must
> not
> + * be HEAP_HOT_UPDATED. 5) If the current tuple is HOT then
> its
> + * predecessor's tuple must be HEAP_HOT_UPDATED.
> + */
>
> This comment needs to be split up into pieces and the pieces need to
> be moved closer to the tests to which they correspond.
>
> Done.


> +  psprintf("unfrozen tuple was
> updated to produce a tuple at offset %u which is not frozen",
>
Shouldn't this say "which is frozen"?
>
> Done.


> + * Not a corruption if current tuple is updated/deleted by a
> + * different transaction, means t_cid will point to cmax
> (that is
> + * command id of deleting transaction) and cid of predecessor
> not
> + * necessarily will be smaller than cid of current tuple.
> t_cid
>
> I think that the next person who reads this code is likely to
> understand that the CIDs of different transactions are numerically
> unrelated. What's less obvious is that if the XID is the same, the
> newer update must have a higher CID.
>
> + * can hold combo command id but we are not worrying here
> since
> + * combo command id of the next updated tuple (if present)
> must be
> + * greater than combo command id of the current tuple. So
> here we
> + * are not checking HEAP_COMBOCID flag and simply doing t_cid
> + * comparison.
>
> I disapprove of ignoring the HEAP_COMBOCID flag. Emitting a message
> claiming that the CID has a certain value when that's actually a combo
> CID is misleading, so at least a different message wording is needed
> in such cases. But it's also not clear to me that the newer update has
> to have a higher combo CID, because combo CIDs can be reused. If you
> have multiple cursors open in the same transaction, the updates can be
> interleaved, and it seems to me that it might be possible for an older
> CID to have created a certain combo CID after a newer CID, and then
> both cursors could update the same page in succession and end up with
> combo CIDs out of numerical order. Unless somebody can make a
> convincing argument that this isn't possible, I think we should just
> skip this check for cases where either tuple has a combo CID.
>
> +if (TransactionIdEquals(pred_xmin, curr_xmin) &&
> +(TransactionIdEquals(curr_xmin, curr_xmax) ||
> + !TransactionIdIsValid(curr_xmax)) && pred_cmin >=
> curr_cmin)
>
> I don't understand the reason for the middle part of this condition --
> TransactionIdEquals(curr_xmin, curr_xmax) ||
> !TransactionIdIsValid(curr_xmax). I suppose the comment is meant to
> explain this, but I still don't get it. If a tuple with XMIN 12345
> CMIN 2 is updated to produce a tuple with XMIN 12345 CMIN 1, that's
> corruption, regardless of what the XMAX of the second tuple may happen
> to be.
>
> As discussed in our last discussion, I am removing 

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Tue, Sep 27, 2022 at 1:35 AM Robert Haas  wrote:

> On Sat, Sep 24, 2022 at 8:45 AM Himanshu Upadhyaya
>  wrote:
> > Here our objective is to validate if both Predecessor's xmin and current
> Tuple's xmin are same then cmin of predecessor must be less than current
> Tuple's cmin. In case when both tuple xmin's are same then I think
> predecessor's t_cid will always hold combo CID.
> > Then either one or both tuple will always have a combo CID and skipping
> this check based on "either tuple has a combo CID" will make this if
> condition to be evaluated to false ''.
>
> Fair point. I think maybe we should just remove the CID validation
> altogether. I thought initially that it would be possible to have a
> newer update with a numerically lower combo CID, but after some
> experimentation I don't see a way to do it. However, it also doesn't
> seem very useful to me to check that the combo CIDs are in ascending
> order. I mean, even if that's not supposed to happen and does anyway,
> there aren't really any enduring consequences, because command IDs are
> ignored completely outside of the transaction that performed the
> operation originally. So even if the combo CIDs were set to completely
> random values, is that really corruption? At most it messes things up
> for the duration of one transaction. And if we just have plain CIDs
> rather than combo CIDs, the same thing is true: they could be totally
> messed up and it wouldn't really matter beyond the lifetime of that
> one transaction.
>
> Also, it would be a lot more tempting to check this if we could check
> it in all cases, but we can't. If a tuple is inserted in transaction
> T1 and ten updated twice in transaction T2, we'll have only one combo
> CID and nothing to compare it against, nor any way to decode what CMIN
> and CMAX it originally represented. And this is probably a pretty
> common type of case.
>
> ok, I will be removing this entire validation of cmin/cid in my next patch.


> >> +if (TransactionIdEquals(pred_xmin, curr_xmin) &&
> >> +(TransactionIdEquals(curr_xmin, curr_xmax) ||
> >> + !TransactionIdIsValid(curr_xmax)) && pred_cmin >=
> curr_cmin)
> >>
> >> I don't understand the reason for the middle part of this condition --
> >> TransactionIdEquals(curr_xmin, curr_xmax) ||
> >> !TransactionIdIsValid(curr_xmax). I suppose the comment is meant to
> >> explain this, but I still don't get it. If a tuple with XMIN 12345
> >> CMIN 2 is updated to produce a tuple with XMIN 12345 CMIN 1, that's
> >> corruption, regardless of what the XMAX of the second tuple may happen
> >> to be.
> >
> > tuple | t_xmin | t_xmax | t_cid | t_ctid |
> tuple_data_split   |
>heap_tuple_infomask_flags
> >
> >
> ---+++---++-+--
> > -
> >  1 |971 |971 | 0 | (0,3)  |
> {"\\x1774657374312020202020","\\x0100"} |
> ("{HEAP_HASVARWIDTH,HEAP_COMBOCID,HEAP_XMIN_COMMITTED,HEAP_XMAX_COMMITTED,HEAP_HOT_UPDATED}",{})
> >  2 |971 |  0 | 1 | (0,2)  |
> {"\\x1774657374322020202020","\\x0200"} |
> ("{HEAP_HASVARWIDTH,HEAP_XMAX_INVALID}",{})
> >  3 |971 |971 | 1 | (0,4)  |
> {"\\x1774657374322020202020","\\x0100"} |
> ("{HEAP_HASVARWIDTH,HEAP_COMBOCID,HEAP_XMIN_COMMITTED,HEAP_XMAX_COMMITTED,HEAP_UPDATED,HEAP_HOT_UPDATED,HEAP_ONLY
> > _TUPLE}",{})
> >  4 |971 |972 | 0 | (0,5)  |
> {"\\x1774657374332020202020","\\x0100"} |
> ("{HEAP_HASVARWIDTH,HEAP_XMIN_COMMITTED,HEAP_UPDATED,HEAP_HOT_UPDATED,HEAP_ONLY_TUPLE}",{})
> >  5 |972 |  0 | 0 | (0,5)  |
> {"\\x1774657374342020202020","\\x0100"} |
> ("{HEAP_HASVARWIDTH,HEAP_XMAX_INVALID,HEAP_UPDATED,HEAP_ONLY_TUPLE}",{})
> >
> > In the above case Tuple 1->3->4 is inserted and updated by xid 971 and
> tuple 4 is next update by xid 972, here t_cid of tuple 4 is 0 where as its
> predecessor's t_cid is 1, because in Tuple 4 t_cid is having command ID of
> deleting transaction(cmax), that is why we need to check xmax of the Tuple.
> >
> > Please correct me if I am missing anything here?
>
> Hmm, I see, so basically you're trying to check whether the CID field
> contains a CMIN as opposed to a CMAX. But I'm not sure this test is
> entirely reliable, because heap_prepare/execute_freeze_tuple() can set
> a tuple's xmax to InvalidTransactionId even after it's had some other
> value, and that won't do anything to the contents of the CID field.
>

ok, Got it, as we are removing this cmin/cid validation so we don't need
any change here.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Tue, Sep 20, 2022 at 6:43 PM Robert Haas  wrote:

> I disapprove of ignoring the HEAP_COMBOCID flag. Emitting a message
> claiming that the CID has a certain value when that's actually a combo
> CID is misleading, so at least a different message wording is needed
> in such cases. But it's also not clear to me that the newer update has
> to have a higher combo CID, because combo CIDs can be reused. If you
> have multiple cursors open in the same transaction, the updates can be
> interleaved, and it seems to me that it might be possible for an older
> CID to have created a certain combo CID after a newer CID, and then
> both cursors could update the same page in succession and end up with
> combo CIDs out of numerical order. Unless somebody can make a
> convincing argument that this isn't possible, I think we should just
> skip this check for cases where either tuple has a combo CID.
>
> Here our objective is to validate if both Predecessor's xmin and current
Tuple's xmin are same then cmin of predecessor must be less than current
Tuple's cmin. In case when both tuple xmin's are same then I think
predecessor's t_cid will always hold combo CID.
Then either one or both tuple will always have a combo CID and skipping
this check based on "either tuple has a combo CID" will make this if
condition to be evaluated to false ''.


> +if (TransactionIdEquals(pred_xmin, curr_xmin) &&
> +(TransactionIdEquals(curr_xmin, curr_xmax) ||
> + !TransactionIdIsValid(curr_xmax)) && pred_cmin >=
> curr_cmin)
>
> I don't understand the reason for the middle part of this condition --
> TransactionIdEquals(curr_xmin, curr_xmax) ||
> !TransactionIdIsValid(curr_xmax). I suppose the comment is meant to
> explain this, but I still don't get it. If a tuple with XMIN 12345
> CMIN 2 is updated to produce a tuple with XMIN 12345 CMIN 1, that's
> corruption, regardless of what the XMAX of the second tuple may happen
> to be.
>

tuple | t_xmin | t_xmax | t_cid | t_ctid |  tuple_data_split
|
heap_tuple_infomask_flags

---+++---++-+--
-
 1 |971 |971 | 0 | (0,3)  |
{"\\x1774657374312020202020","\\x0100"} |
("{HEAP_HASVARWIDTH,HEAP_COMBOCID,HEAP_XMIN_COMMITTED,HEAP_XMAX_COMMITTED,HEAP_HOT_UPDATED}",{})
 2 |971 |  0 | 1 | (0,2)  |
{"\\x1774657374322020202020","\\x0200"} |
("{HEAP_HASVARWIDTH,HEAP_XMAX_INVALID}",{})
 3 |971 |971 | 1 | (0,4)  |
{"\\x1774657374322020202020","\\x0100"} |
("{HEAP_HASVARWIDTH,HEAP_COMBOCID,HEAP_XMIN_COMMITTED,HEAP_XMAX_COMMITTED,HEAP_UPDATED,HEAP_HOT_UPDATED,HEAP_ONLY
_TUPLE}",{})
 4 |971 |972 | 0 | (0,5)  |
{"\\x1774657374332020202020","\\x0100"} |
("{HEAP_HASVARWIDTH,HEAP_XMIN_COMMITTED,HEAP_UPDATED,HEAP_HOT_UPDATED,HEAP_ONLY_TUPLE}",{})
 5 |972 |  0 | 0 | (0,5)  |
{"\\x1774657374342020202020","\\x0100"} |
("{HEAP_HASVARWIDTH,HEAP_XMAX_INVALID,HEAP_UPDATED,HEAP_ONLY_TUPLE}",{})

In the above case Tuple 1->3->4 is inserted and updated by xid 971 and
tuple 4 is next update by xid 972, here t_cid of tuple 4 is 0 where as its
predecessor's t_cid is 1, because in Tuple 4 t_cid is having command ID of
deleting transaction(cmax), that is why we need to check xmax of the Tuple.

Please correct me if I am missing anything here?
-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Mon, Sep 19, 2022 at 10:04 PM Aleksander Alekseev <
aleksan...@timescale.com> wrote:

> Hi Himanshu,
>
> > I have changed this in the attached patch.
>
> If it's not too much trouble could you please base your changes on v4
> that I submitted? I put some effort into writing a proper commit
> message, editing the comments, etc. The easiest way of doing this is
> using `git am` and `git format-patch`.
>
> Please find it attached.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From 3f3290ffb857117385f79f85aa599c588340924b Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Tue, 20 Sep 2022 14:23:31 +0530
Subject: [PATCH v5] Implement HOT chain validation in verify_heapam()

Himanshu Upadhyaya, reviewed by Robert Haas, Aleksander Alekseev

Discussion: https://postgr.es/m/CAPF61jBBR2-iE-EmN_9v0hcQEfyz_17e5Lbb0%2Bu2%3D9ukA9sWmQ%40mail.gmail.com
---
 contrib/amcheck/verify_heapam.c | 214 
 1 file changed, 214 insertions(+)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..e90e1ef2f3 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -399,6 +399,9 @@ verify_heapam(PG_FUNCTION_ARGS)
 	for (ctx.blkno = first_block; ctx.blkno <= last_block; ctx.blkno++)
 	{
 		OffsetNumber maxoff;
+		OffsetNumber predecessor[MaxOffsetNumber] = {0};
+		OffsetNumber successor[MaxOffsetNumber] = {0};
+		bool		lp_valid[MaxOffsetNumber] = {false};
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -433,6 +436,8 @@ verify_heapam(PG_FUNCTION_ARGS)
 		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
 			 ctx.offnum = OffsetNumberNext(ctx.offnum))
 		{
+			OffsetNumber nextoffnum;
+
 			ctx.itemid = PageGetItemId(ctx.page, ctx.offnum);
 
 			/* Skip over unused/dead line pointers */
@@ -469,6 +474,12 @@ verify_heapam(PG_FUNCTION_ARGS)
 	report_corruption(,
 	  psprintf("line pointer redirection to unused item at offset %u",
 			   (unsigned) rdoffnum));
+
+/*
+ * make entry in successor array, redirected tuple will be
+ * validated at the time when we loop over successor array
+ */
+successor[ctx.offnum] = rdoffnum;
 continue;
 			}
 
@@ -504,9 +515,205 @@ verify_heapam(PG_FUNCTION_ARGS)
 			/* It should be safe to examine the tuple's header, at least */
 			ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page, ctx.itemid);
 			ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
+			lp_valid[ctx.offnum] = true;
 
 			/* Ok, ready to check this next tuple */
 			check_tuple();
+
+			/*
+			 * Add the data to the successor array. It will be used later to
+			 * generate the predecessor array.
+			 *
+			 * We need to access the tuple's header to populate the
+			 * predecessor array. However the tuple is not necessarily sanity
+			 * checked yet so delaying construction of predecessor array until
+			 * all tuples are sanity checked.
+			 */
+			nextoffnum = ItemPointerGetOffsetNumber(&(ctx.tuphdr)->t_ctid);
+			if (ItemPointerGetBlockNumber(&(ctx.tuphdr)->t_ctid) == ctx.blkno &&
+nextoffnum != ctx.offnum)
+			{
+successor[ctx.offnum] = nextoffnum;
+			}
+		}
+
+		/*
+		 * Loop over offset and populate predecessor array from all entries
+		 * that are present in successor array.
+		 */
+		ctx.attnum = -1;
+		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
+			 ctx.offnum = OffsetNumberNext(ctx.offnum))
+		{
+			ItemId		curr_lp;
+			ItemId		next_lp;
+			HeapTupleHeader curr_htup;
+			HeapTupleHeader next_htup;
+			TransactionId curr_xmax;
+			TransactionId next_xmin;
+
+			OffsetNumber nextoffnum = successor[ctx.offnum];
+
+			curr_lp = PageGetItemId(ctx.page, ctx.offnum);
+			if (nextoffnum == 0)
+			{
+/*
+ * This is either the last updated tuple in the chain or a
+ * corruption raised for this tuple.
+ */
+continue;
+			}
+			if (ItemIdIsRedirected(curr_lp) && lp_valid[nextoffnum])
+			{
+next_lp = PageGetItemId(ctx.page, nextoffnum);
+next_htup = (HeapTupleHeader) PageGetItem(ctx.page, next_lp);;
+if (!HeapTupleHeaderIsHeapOnly(next_htup))
+{
+	report_corruption(,
+	  psprintf("redirected tuple at line pointer offset %u is not heap only tuple",
+			   (unsigned) nextoffnum));
+}
+if ((next_htup->t_infomask & HEAP_UPDATED) == 0)
+{
+	report_corruption(,
+	  psprintf("redirected tuple at line pointer offset %u is not heap updated tuple",
+			   (unsigned) nextoffnum));
+}
+continue;
+			}
+
+			/*
+			 * Add a line pointer offset to the predecessor array. 1) If xmax
+			 * is matching with xmin of next tuple (reaching via its t_ctid).
+			 * 2) If the next tuple is in the same page. Raise corruption if
+			 * we have two tuples having the same predecessor.
+			 *
+			 * We add the offset to the predecessor array irrespective of the
+		

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Mon, Sep 19, 2022 at 8:27 PM Aleksander Alekseev <
aleksan...@timescale.com> wrote:

> Hi Himanshu,
>
> > Done, updated in the v3 patch.
>
> Thanks for the updated patch.
>
> Here is v4 with fixed compiler warnings and some minor tweaks from me.
>
> I didn't put too much thought into the algorithm but I already see
> something strange. At verify_heapam.c:553 you declared curr_xmax and
> next_xmin. However the variables are not used/initialized until you
> do:
>
> ```
> if (lp_valid[nextoffnum] && lp_valid[ctx.offnum] &&
> TransactionIdIsValid(curr_xmax) &&
> TransactionIdEquals(curr_xmax, next_xmin)) {
> /* ... */
> ```
>
> In v4 I elected to initialize both curr_xmax and next_xmin with
> InvalidTransactionId for safety and in order to silence the compiler
> but still there is no way this condition can succeed.
>
> Please make sure there is no logic missing.
>
>
Hi Aleksander,

Thanks for sharing the feedback,
It's my mistake, sorry about that, I was trying to merge two if conditions
and forgot to move the initialization part for xmin and xmax. Now I think
that it will be good to have nested if, and have an inner if condition to
test xmax and xmin matching. This way we can retrieve and populate xmin and
xmax when it is actually required for the inner if condition.
I have changed this in the attached patch.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From 1a51c544c5c9a14441831f5299b9d6fe275fbf53 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Mon, 19 Sep 2022 21:44:34 +0530
Subject: [PATCH v4] HOT chain validation in verify_heapam

---
 contrib/amcheck/verify_heapam.c | 211 
 1 file changed, 211 insertions(+)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..2b9244ca69 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -399,6 +399,9 @@ verify_heapam(PG_FUNCTION_ARGS)
 	for (ctx.blkno = first_block; ctx.blkno <= last_block; ctx.blkno++)
 	{
 		OffsetNumber maxoff;
+		OffsetNumber predecessor[MaxOffsetNumber] = {0};
+		OffsetNumber successor[MaxOffsetNumber] = {0};
+		bool		lp_valid[MaxOffsetNumber] = {false};
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -433,6 +436,8 @@ verify_heapam(PG_FUNCTION_ARGS)
 		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
 			 ctx.offnum = OffsetNumberNext(ctx.offnum))
 		{
+			OffsetNumber nextoffnum;
+
 			ctx.itemid = PageGetItemId(ctx.page, ctx.offnum);
 
 			/* Skip over unused/dead line pointers */
@@ -469,6 +474,12 @@ verify_heapam(PG_FUNCTION_ARGS)
 	report_corruption(,
 	  psprintf("line pointer redirection to unused item at offset %u",
 			   (unsigned) rdoffnum));
+
+/*
+ * make entry in successor array, redirected tuple will be
+ * validated at the time when we loop over successor array
+ */
+successor[ctx.offnum] = rdoffnum;
 continue;
 			}
 
@@ -504,9 +515,202 @@ verify_heapam(PG_FUNCTION_ARGS)
 			/* It should be safe to examine the tuple's header, at least */
 			ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page, ctx.itemid);
 			ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
+			lp_valid[ctx.offnum] = true;
 
 			/* Ok, ready to check this next tuple */
 			check_tuple();
+
+			/*
+			 * Now add data to successor array, it will later be used to
+			 * generate the predecessor array. We need to access the tuple's
+			 * header to populate the predecessor array but tuple is not
+			 * necessarily sanity checked yet so delaying construction of
+			 * predecessor array until all tuples are sanity checked.
+			 */
+			nextoffnum = ItemPointerGetOffsetNumber(&(ctx.tuphdr)->t_ctid);
+			if (ItemPointerGetBlockNumber(&(ctx.tuphdr)->t_ctid) == ctx.blkno &&
+nextoffnum != ctx.offnum)
+			{
+successor[ctx.offnum] = nextoffnum;
+			}
+		}
+
+		/*
+		 * Loop over offset and populate predecessor array from all entries
+		 * that are present in successor array.
+		 */
+		ctx.attnum = -1;
+		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
+			 ctx.offnum = OffsetNumberNext(ctx.offnum))
+		{
+			ItemId		curr_lp;
+			ItemId		next_lp;
+			HeapTupleHeader curr_htup;
+			HeapTupleHeader next_htup;
+			TransactionId curr_xmax;
+			TransactionId next_xmin;
+			OffsetNumber nextoffnum = successor[ctx.offnum];
+
+			curr_lp = PageGetItemId(ctx.page, ctx.offnum);
+			if (nextoffnum == 0)
+			{
+/*
+ * Either last updated tuple in chain or corruption raised for
+ * this tuple.
+ */
+continue;
+			}
+			if (ItemIdIsRedirected(curr_lp) && lp_valid[nextoffnum])
+			{
+next_lp = PageGetItemId(ctx.page, nextoffnum);
+next_htup = (HeapTupleHeader) PageGetItem(ctx.page, next_lp);;
+if (!HeapTupleHeaderIsHeapOnly(next_htu

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Wed, Sep 7, 2022 at 12:11 AM Robert Haas  wrote:

>
> I think the check should be written like this:
>
> !TransactionIdEquals(pred_xmin, curr_xmin) &&
> !TransctionIdDidCommit(pred_xmin)
>
> The TransactionIdEquals check should be done first for the reason you
> state: it's cheaper.
>
> I think that we shouldn't be using TransactionIdDidAbort() at all,
> because it can sometimes return false even when the transaction
> actually did abort. See test_lockmode_for_conflict() and
> TransactionIdIsInProgress() for examples of logic that copes with
> this. What's happening here is that TransactionIdDidAbort doesn't ever
> get called if the system crashes while a transaction is running. So we
> can use TransactionIdDidAbort() only as an optimization: if it returns
> true, the transaction is definitely aborted, but if it returns false,
> we have to check whether it's still running. If not, it aborted
> anyway.
>
> TransactionIdDidCommit() does not have the same issue. A transaction
> can abort without updating CLOG if the system crashes, but it can
> never commit without updating CLOG. If the transaction didn't commit,
> then it is either aborted or still in progress (and we don't care
> which, because neither is an error here).
>
> As to whether the existing formulation of the test has an error
> condition, you're generally right that we should test
> TransactionIdIsInProgress() before TransactionIdDidCommit/Abort,
> because we during commit or abort, we first set the status in CLOG -
> which is queried by TransactionIdDidCommit/Abort - and only afterward
> update the procarray - which is queried by TransactionIdIsInProgress.
> So normally TransactionIdIsInProgress should be checked first, and
> TransactionIdDidCommit/Abort should only be checked if it returns
> false, at which point we know that the return values of the latter
> calls can't ever change. Possibly there is an argument for including
> the TransactionIdInProgress check here too:
>
> !TransactionIdEquals(pred_xmin, curr_xmin) &&
> (TransactionIdIsInProgress(pred_xmin) ||
> !TransctionIdDidCommit(pred_xmin))
>
> ...but I don't think it could change the answer. Most places that
> check TransactionIdIsInProgress() first are concerned with MVCC
> semantics, and here we are not. I think the only effects of including
> or excluding the TransactionIdIsInProgress() test are (1) performance,
> in that searching the procarray might avoid expense if it's cheaper
> than searching clog, or add expense if the reverse is true and (2)
> slightly changing the time at which we're first able to detect this
> form of corruption. I am inclined to prefer the simpler form of the
> test without TransactionIdIsInProgress() unless someone can say why we
> shouldn't go that route.
>
> Done, updated in the v3 patch.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Wed, Sep 7, 2022 at 2:49 AM Robert Haas  wrote:

>
> But here's one random idea: add a successor[] array and an lp_valid[]
> array. In the first loop, set lp_valid[offset] = true if it passes the
> check_lp() checks, and set successor[A] = B if A redirects to B or has
> a CTID link to B, without matching xmin/xmax. Then, in a second loop,
> iterate over the successor[] array. If successor[A] = B && lp_valid[A]
> && lp_valid[B], then check whether A.xmax = B.xmin; if so, then
> complain if predecessor[B] is already set, else set predecessor[B] =
> A. Then, in the third loop, iterate over the predecessor array just as
> you're doing now. Then it's clear that we do the lp_valid checks
> exactly once for every offset that might need them, and in order. And
> it's also clear that the predecessor-based checks can never happen
> unless the lp_valid checks passed for both of the offsets involved.
>
>
ok, I have introduced a new approach to first construct a successor array
and then loop over the successor array to construct a predecessor array.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From 4a32d7cbdec0d090e99a9a58cb4deb5cb4c03aef Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Mon, 19 Sep 2022 13:50:47 +0530
Subject: [PATCH v3] HOT chain validation in verify_heapam

---
 contrib/amcheck/verify_heapam.c | 209 
 1 file changed, 209 insertions(+)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..3da4c09665 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -399,6 +399,9 @@ verify_heapam(PG_FUNCTION_ARGS)
 	for (ctx.blkno = first_block; ctx.blkno <= last_block; ctx.blkno++)
 	{
 		OffsetNumber maxoff;
+		OffsetNumber predecessor[MaxOffsetNumber] = {0};
+		OffsetNumber successor[MaxOffsetNumber] = {0};
+		bool		lp_valid[MaxOffsetNumber] = {false};
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -433,6 +436,8 @@ verify_heapam(PG_FUNCTION_ARGS)
 		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
 			 ctx.offnum = OffsetNumberNext(ctx.offnum))
 		{
+			OffsetNumber nextoffnum;
+
 			ctx.itemid = PageGetItemId(ctx.page, ctx.offnum);
 
 			/* Skip over unused/dead line pointers */
@@ -469,6 +474,12 @@ verify_heapam(PG_FUNCTION_ARGS)
 	report_corruption(,
 	  psprintf("line pointer redirection to unused item at offset %u",
 			   (unsigned) rdoffnum));
+
+/*
+ * make entry in successor array, redirected tuple will be
+ * validated at the time when we loop over successor array
+ */
+successor[ctx.offnum] = rdoffnum;
 continue;
 			}
 
@@ -504,9 +515,200 @@ verify_heapam(PG_FUNCTION_ARGS)
 			/* It should be safe to examine the tuple's header, at least */
 			ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page, ctx.itemid);
 			ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
+			lp_valid[ctx.offnum] = true;
 
 			/* Ok, ready to check this next tuple */
 			check_tuple();
+
+			/*
+			 * Now add data to successor array, it will later be used to
+			 * generate the predecessor array. We need to access the tuple's
+			 * header to populate the predecessor array but tuple is not
+			 * necessarily sanity checked yet so delaying construction of
+			 * predecessor array until all tuples are sanity checked.
+			 */
+			nextoffnum = ItemPointerGetOffsetNumber(&(ctx.tuphdr)->t_ctid);
+			if (ItemPointerGetBlockNumber(&(ctx.tuphdr)->t_ctid) == ctx.blkno &&
+nextoffnum != ctx.offnum)
+			{
+successor[ctx.offnum] = nextoffnum;
+			}
+		}
+
+		/*
+		 * Loop over offset and populate predecessor array from all entries
+		 * that are present in successor array.
+		 */
+		ctx.attnum = -1;
+		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
+			 ctx.offnum = OffsetNumberNext(ctx.offnum))
+		{
+			ItemId		curr_lp;
+			ItemId		next_lp;
+			HeapTupleHeader curr_htup;
+			HeapTupleHeader next_htup;
+			TransactionId curr_xmax;
+			TransactionId next_xmin;
+			OffsetNumber nextoffnum = successor[ctx.offnum];
+
+			curr_lp = PageGetItemId(ctx.page, ctx.offnum);
+			if (nextoffnum == 0)
+			{
+/*
+ * Either last updated tuple in chain or corruption raised for
+ * this tuple.
+ */
+continue;
+			}
+			if (ItemIdIsRedirected(curr_lp) && lp_valid[nextoffnum])
+			{
+next_lp = PageGetItemId(ctx.page, nextoffnum);
+next_htup = (HeapTupleHeader) PageGetItem(ctx.page, next_lp);;
+if (!HeapTupleHeaderIsHeapOnly(next_htup))
+{
+	report_corruption(,
+	  psprintf("redirected tuple at line pointer offset %u is not heap only tuple",
+			   (unsigned) nextoffnum));
+}
+if ((next_htup->t_infomask & HEAP_UPDATED) == 0)
+{
+	report_corruption(,
+	  psprintf("redirected tuple at line pointer offset %u is not heap updated tuple",
+		

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

On Wed, Sep 7, 2022 at 2:49 AM Robert Haas  wrote:

>
> But here's one random idea: add a successor[] array and an lp_valid[]
> array. In the first loop, set lp_valid[offset] = true if it passes the
> check_lp() checks, and set successor[A] = B if A redirects to B or has
> a CTID link to B, without matching xmin/xmax. Then, in a second loop,
> iterate over the successor[] array. If successor[A] = B && lp_valid[A]
> && lp_valid[B], then check whether A.xmax = B.xmin; if so, then
> complain if predecessor[B] is already set, else set predecessor[B] =
> A. Then, in the third loop, iterate over the predecessor array just as
> you're doing now. Then it's clear that we do the lp_valid checks
> exactly once for every offset that might need them, and in order. And
> it's also clear that the predecessor-based checks can never happen
> unless the lp_valid checks passed for both of the offsets involved.
>
>
>
Approach of introducing a successor array is good but I see one overhead
with having both successor and predecessor array, that is, we will traverse
each offset on page thrice(one more for original loop on offset) and with
each offset we have to retrieve
/reach an ItemID(PageGetItemId) and Item(PageGetItem) itself. This is not
much overhead as they are all preprocessors but there will be some overhead.
How about having new array(initialised with LP_NOT_CHECKED) of enum
LPStatus as below

typedef enum LPStatus
{
LP_NOT_CHECKED,
LP_VALID,
LP_NOT_VALID
}LPStatus;

and validating and setting with proper status at three places
1) while validating Redirect Tuple
2) while validating populating predecessor array and
3) at original place  of "sanity check"


something like:
" if (lpStatus[rdoffnum] == LP_NOT_CHECKED)
{
ctx.offnum = rdoffnum;
if (!check_lp(,
ItemIdGetLength(rditem), ItemIdGetOffset(rditem)))
{
lpStatus[rdoffnum] =
LP_NOT_VALID;
continue;
}
lpStatus[rdoffnum] = LP_VALID;
}
else if (lpStatus[rdoffnum] == LP_NOT_VALID)
        continue;
"



thoughts?
-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

amp;&
> +HeapTupleHeaderIsHotUpdated(curr_htup))
> +{
> +report_corruption(,
> +psprintf("Current tuple at offset %u is
> HOT but is last tuple in the HOT chain.",
> +(unsigned) ctx.offnum));
> +}
>
> This check has nothing to do with the predecessor[] array, so it seems
> like it belongs in check_tuple() rather than here. Also, the message
> is rather confused, because the test is checking whether the tuple has
> been HOT-updated, while the message is talking about whether the tuple
> was *itself* created by a HOT update. Also, when we're dealing with
> corruption, statements like "is last tuple in the HOT chain" are
> pretty ambiguous. Also, isn't this an issue for both HOT-updated
> tuples and also just regular updated tuples? i.e. maybe what we should
> be complaining about here is something like "tuple has been updated,
> but xmax is 0" and then make the test check exactly that.
>

Moved to check_tuple_header. This should be applicable for both HOT and
normal updates but even the last updated tuple in the normal update is
HEAP_UPDATED so not sure how we can apply this check for a normal update?

+if (!HeapTupleHeaderIsHotUpdated(pred_htup) &&
> +HeapTupleHeaderIsHeapOnly(pred_htup) &&
> +HeapTupleHeaderIsHeapOnly(curr_htup))
> +{
> +report_corruption(,
> +psprintf("Current tuple at offset %u is
> HOT but it is next updated tuple of last Tuple in HOT chain.",
> +(unsigned) ctx.offnum));
> +}
>
> Three if-statements up, you tested two out of these three conditions
> and complained if they were met. So any time this fires, that will
> have also fired.
>

Yes, the above condition is not required. Now removed.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From 5adc1d3c6c4526b723a114ca00eacdea20143861 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Tue, 6 Sep 2022 15:59:40 +0530
Subject: [PATCH v2] HOT chain validation in verify_heapam.

---
 contrib/amcheck/verify_heapam.c | 228 +---
 1 file changed, 206 insertions(+), 22 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..cc69ccb239 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -158,6 +158,7 @@ static void check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
 static bool check_tuple_attribute(HeapCheckContext *ctx);
 static void check_toasted_attribute(HeapCheckContext *ctx,
 	ToastedAttribute *ta);
+static bool check_lp(HeapCheckContext *ctx, uint16 lp_len, uint16 lp_off);
 
 static bool check_tuple_header(HeapCheckContext *ctx);
 static bool check_tuple_visibility(HeapCheckContext *ctx);
@@ -399,6 +400,7 @@ verify_heapam(PG_FUNCTION_ARGS)
 	for (ctx.blkno = first_block; ctx.blkno <= last_block; ctx.blkno++)
 	{
 		OffsetNumber maxoff;
+		OffsetNumber predecessor[MaxOffsetNumber] = {0};
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -433,6 +435,10 @@ verify_heapam(PG_FUNCTION_ARGS)
 		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
 			 ctx.offnum = OffsetNumberNext(ctx.offnum))
 		{
+			OffsetNumber nextTupOffnum;
+			HeapTupleHeader htup;
+			OffsetNumber	currOffnum = ctx.offnum;
+
 			ctx.itemid = PageGetItemId(ctx.page, ctx.offnum);
 
 			/* Skip over unused/dead line pointers */
@@ -469,44 +475,178 @@ verify_heapam(PG_FUNCTION_ARGS)
 	report_corruption(,
 	  psprintf("line pointer redirection to unused item at offset %u",
 			   (unsigned) rdoffnum));
+
+ctx.offnum = rdoffnum;
+if (!check_lp(, ItemIdGetLength(rditem), ItemIdGetOffset(rditem)))
+	continue;
+ctx.offnum = currOffnum;
+
+htup = (HeapTupleHeader) PageGetItem(ctx.page, rditem);
+if (!HeapTupleHeaderIsHeapOnly(htup))
+{
+	report_corruption(,
+	  psprintf("redirected tuple at line pointer offset %u is not heap only tuple",
+			   (unsigned) rdoffnum));
+}
+if ((htup->t_infomask & HEAP_UPDATED) == 0)
+{
+	report_corruption(,
+	  psprintf("redirected tuple at line pointer offset %u is not heap updated tuple",
+			   (unsigned) rdoffnum));
+}
 continue;
 			}
 
 			/* Sanity-check the line pointer's offset and length values */
 			ctx.lp_len = ItemIdGetLength(ctx.itemid);
 			ctx.lp_off = ItemIdGetOffset(ctx.itemid);
+			/*
+			 * If offnum is already present in predecessor array means it was
+			 * previously sanity-checked.
+			 */
+			if (predecessor[ctx.offnum] == 0 && !check_lp(, ctx.lp_len, ctx.lp_off))
+continue;
+
+			/* It should be safe to examine the

HOT chain validation in verify_heapam()

[Himanshu Upadhyaya]

Hi,

On Mon, Apr 4, 2022 at 11:46 PM Andres Freund  wrote:

>
> I think there's a few more things that'd be good to check. For example
> amcheck
> doesn't verify that HOT chains are reasonable, which can often be spotted
> looking at an individual page. Which is a bit unfortunate, given how many
> bugs
> we had in that area.
>
> Stuff to check around that:
> - target of redirect has HEAP_ONLY_TUPLE, HEAP_UPDATED set
> - In a valid ctid chain within a page (i.e. xmax = xmin):
>   - tuples have HEAP_UPDATED set
>   - HEAP_ONLY_TUPLE / HEAP_HOT_UPDATED matches across chains elements


(I changed the subject because the attached patch is related to HOT chain
validation).

Please find attached the patch with the above idea of HOT chain's
validation(within a Page) and a few more validation as below.

* If the predecessor’s xmin is aborted or in progress, the current tuples
xmin should be aborted or in progress respectively. Also, Both xmin must be
equal.
* If the predecessor’s xmin is not frozen, then-current tuple’s shouldn’t
be either.
* If the predecessor’s xmin is equal to the current tuple’s xmin, the
current tuple’s cmin should be greater than the predecessor’s xmin.
* If the current tuple is not HOT then its predecessor’s tuple must not be
HEAP_HOT_UPDATED.
* If the current Tuple is HOT then its predecessor’s tuple must be
HEAP_HOT_UPDATED and vice-versa.
* If xmax is 0, which means it's the last tuple in the chain, then it must
not be HEAP_HOT_UPDATED.
* If the current tuple is the last tuple in the HOT chain then the next
tuple should not be HOT.

I am looking into the process of adding the TAP test for these changes and
finding a way to corrupt a page in the tap test. Will try to include these
test cases in my Upcoming version of the patch.

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From f3ae2f783a255109655cade770ebc74e01aef34c Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Thu, 18 Aug 2022 13:20:51 +0530
Subject: [PATCH v1] HOT chain validation in verify_heapam.

---
 contrib/amcheck/verify_heapam.c | 144 
 1 file changed, 144 insertions(+)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..ae2c100de2 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -399,6 +399,7 @@ verify_heapam(PG_FUNCTION_ARGS)
 	for (ctx.blkno = first_block; ctx.blkno <= last_block; ctx.blkno++)
 	{
 		OffsetNumber maxoff;
+		OffsetNumber predecessor[MaxOffsetNumber] = {0};
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -433,6 +434,9 @@ verify_heapam(PG_FUNCTION_ARGS)
 		for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
 			 ctx.offnum = OffsetNumberNext(ctx.offnum))
 		{
+			OffsetNumber nextTupOffnum;
+			HeapTupleHeader htup;
+
 			ctx.itemid = PageGetItemId(ctx.page, ctx.offnum);
 
 			/* Skip over unused/dead line pointers */
@@ -469,6 +473,13 @@ verify_heapam(PG_FUNCTION_ARGS)
 	report_corruption(,
 	  psprintf("line pointer redirection to unused item at offset %u",
 			   (unsigned) rdoffnum));
+
+htup = (HeapTupleHeader) PageGetItem(ctx.page, rditem);
+if (!(HeapTupleHeaderIsHeapOnly(htup) && htup->t_infomask & HEAP_UPDATED))
+	report_corruption(,
+	  psprintf("Redirected Tuple at line pointer offset %u is not HEAP_ONLY_TUPLE or HEAP_UPDATED tuple",
+			   (unsigned) rdoffnum));
+
 continue;
 			}
 
@@ -507,6 +518,139 @@ verify_heapam(PG_FUNCTION_ARGS)
 
 			/* Ok, ready to check this next tuple */
 			check_tuple();
+			/*
+			 * Add line pointer offset to predecessor array.
+			 * 1) If xmax is matching with xmin of next Tuple(reaching via its t_ctid).
+			 * 2) If next tuple is in the same page.
+			 * Raise corruption if:
+			 * We have two tuples having same predecessor.
+			 *
+			 * We add offset to predecessor irrespective of transaction(t_xmin) status. We will
+			 * do validation related to transaction status(and also all other validations)
+			 * when we loop over predecessor array.
+			 */
+			nextTupOffnum = ItemPointerGetOffsetNumber(&(ctx.tuphdr)->t_ctid);
+			if (ItemPointerGetBlockNumber(&(ctx.tuphdr)->t_ctid) == ctx.blkno &&
+nextTupOffnum != ctx.offnum)
+			{
+TransactionId currTupXmax;
+ItemId lp;
+
+if (predecessor[nextTupOffnum] != 0)
+{
+	report_corruption(,
+psprintf("Tuple at offset %u is reachable from two or more updated tuple",
+	(unsigned) nextTupOffnum));
+	continue;
+}
+currTupXmax = HeapTupleHeaderGetUpdateXid(ctx.tuphdr);
+lp   = PageGetItemId(ctx.page, nextTupOffnum);
+
+htup = (HeapTupleHeader) PageGetItem(ctx.page, lp);
+
+if (TransactionIdIsValid(currTupXmax) &&
+	TransactionIdEquals(HeapTupleHeaderGetXmin(htup), currTupXmax))
+{
+	predecessor[nextTupOffnum] = ctx.offnum;
+}
+/* No

Re: SQL/JSON: JSON_TABLE

[Himanshu Upadhyaya]

On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan  wrote:
>
>
> rebased with some review comments attended to.

I am in process of reviewing these patches, initially, have started
with 0002-JSON_TABLE-v55.patch.
Tested many different scenarios with various JSON messages and these
all are working as expected. Just one question on the below output.

‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
(a int PATH '$.a' ERROR ON EMPTY)) jt;
 a
---

(1 row)

‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
(a int PATH '$.a' ERROR ON ERROR)) jt;
 a
---

(1 row)

is not "ERROR ON ERROR" is expected to give error?

There are a few minor comments on the patch:
1)
Few Typo
+  Sibling columns are joined using
+  FULL OUTER JOIN ON FALSE, so that both
parent and child
+  rows are included into the output, with NULL values inserted
+  into both child and parrent columns for all missing values.

Parrent should be parent.

+
+ Gerenates a column and inserts a boolean item into each row of
this column.
+
Gerenates should be Generates.

+
+ Extracts SQL/JSON items from nested levels of the row pattern,
+ gerenates one or more columns as defined by the COLUMNS
+ subclause, and inserts the extracted SQL/JSON items into each
row of these columns.
+ The json_table_column expression in the
+ COLUMNS subclause uses the same syntax as in the
+ parent COLUMNS clause.
+

Gerenates should be Generates.

+/*-
+ *
+ * parse_jsontable.c
+ *   pasring of JSON_TABLE

pasring should be parsing.

2) Albhabatic include.
+
+#include "postgres.h"
+
+#include "miscadmin.h"
+
include files are not in alphabetic order.

3)
+-- JSON_TABLE: nested paths and plans
+-- Should fail (column names anf path names shall be distinct)
+SELECT * FROM JSON_TABLE(
+   jsonb '[]', '$'
+   COLUMNS (
+   a int,
+   b text,
+   a jsonb
+   )
+) jt;
+ERROR:  duplicate JSON_TABLE column name: a
+HINT:  JSON_TABLE path names and column names shall be distinct from
one another

HINT is not much relevant, can't we simply say "JSON_TABLE column
names should be distinct from one another"?

4)
@@ -4837,6 +4844,7 @@ ExecEvalJsonExprSubtrans(JsonFunc func, ExprEvalStep *op,
/* Want to execute expressions inside function's memory context */
MemoryContextSwitchTo(oldcontext);

+

we can remove this empty line.

5)
/*
 * The production for a qualified relation name has to exactly match the
 * production for a qualified func_name, because in a FROM clause we cannot
 * tell which we are parsing until we see what comes after it ('(' for a
 * func_name, something else for a relation). Therefore we allow 'indirection'
 * which may contain subscripts, and reject that case in the C code.
 */

I think the sentence "because in a FROM clause we cannot
 * tell which we are parsing..." should be changed to "because in a
FROM clause we cannot
 * tell what we are parsing "

6)
@@ -696,7 +696,7 @@ transformRangeTableFunc(ParseState *pstate,
RangeTableFunc *rtf)
char  **names;
int colno;

-   /* Currently only XMLTABLE is supported */

can we change(and not remove) the comment to "/* Currently only
XMLTABLE and JSON_TABLE is supported */"

7)
/*
 * TableFunc - node for a table function, such as XMLTABLE.
 *
 * Entries in the ns_names list are either String nodes containing
 * literal namespace names, or NULL pointers to represent DEFAULT.
 */
typedef struct TableFunc

can we change the comment to "...such as XMLTABLE or JSON_TABLE."?

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: SQL/JSON: functions

[Himanshu Upadhyaya]

On Tue, Jan 4, 2022 at 7:32 PM Andrew Dunstan  wrote:

I have one general question on the below scenario.
CREATE TABLE T (Id INTEGER PRIMARY KEY,Jcol CHARACTER VARYING ( 5000
)CHECK ( Jcol IS JSON ) );
insert into T values (1,323);
 ORACLE is giving an error(check constraint...violated ORA-06512) for
the above insert but Postgres is allowing it, however is not related
to this patch but just thinking if this is expected.

‘postgres[22198]=#’SELECT * FROM T WHERE Jcol IS JSON;
 id | jcol
+--
  1 | 323
How come number 323 is the valid json?

Few comments/doubts on 0003-IS-JSON-predicate-v59.patch and
0004-SQL-JSON-query-functions-v59.patch patch:
1) I am not able to find a case where "IS JSON" and "IS JSON VALUE"
gives a different result, is they intended to give the same result(and
two are replaceably used) when applied on any input.

2) Not sure why we return true for the below query?
+-- extension: boolean expressions
+SELECT JSON_EXISTS(jsonb '1', '$ > 2');
+ json_exists
+-
+ t
+(1 row)

3)
+-- Strict mode with ERROR on ERROR clause
+SELECT JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' ERROR ON ERROR);
+ERROR: Invalid SQL/JSON subscript

The above example in documentation is not actually matching when I am
trying to run with the patch as below.
‘postgres[28411]=#’SELECT JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict
$.a[5]' ERROR ON ERROR);
ERROR:  22033: jsonpath array subscript is out of bounds
LOCATION:  executeItemOptUnwrapTarget, jsonpath_exec.c:769

+SELECT JSON_VALUE('"123.45"', '$' RETURNING float);
+ json_value
+
+ 123.45
+(1 row)

Above is also not matching:
‘postgres[28411]=#’SELECT JSON_VALUE('"123.45"', '$' RETURNING float);
ERROR:  0A000: JSON_VALUE() is not yet implemented for json type
LINE 1: SELECT JSON_VALUE('"123.45"', '$' RETURNING float);

There is more such example that does not actually produce the same
result when we try to run after applying this patch, seems like we
just need to update the documentation with regards to our new patch.
+SELECT JSON_VALUE(jsonb '[1,2]', 'strict $[*]' ERROR ON ERROR);
+ERROR: more than one SQL/JSON item

‘postgres[28411]=#’SELECT JSON_VALUE(jsonb '[1,2]', 'strict $[*]'
ERROR ON ERROR);
ERROR:  22034: JSON path expression in JSON_VALUE should return
singleton scalar item

4)
index f46786231e..c1951c1caf 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -28,6 +28,7 @@
 #include "catalog/pg_type.h"
 #include "executor/executor.h"
 #include "executor/functions.h"
+#include "executor/execExpr.h"
 #include "funcapi.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"

can we adjust the include file in the alphabetic order please?

5)
+SELECT
+   JSON_QUERY(js, '$'),
+   JSON_QUERY(js, '$' WITHOUT WRAPPER),
+   JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER),
+   JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER),
+   JSON_QUERY(js, '$' WITH ARRAY WRAPPER)
+FROM
+   (VALUES
+   (jsonb 'null'),
+   ('12.3'),
+   ('true'),
+   ('"aaa"'),
+   ('[1, null, "2"]'),
+   ('{"a": 1, "b": [2]}')
+   ) foo(js);
+ json_query | json_query | json_query |
json_query  |  json_query
++++--+--
+ null   | null   | [null] |
[null]   | [null]
+ 12.3   | 12.3   | [12.3] |
[12.3]   | [12.3]
+ true   | true   | [true] |
[true]   | [true]
+ "aaa"  | "aaa"  | ["aaa"]|
["aaa"]  | ["aaa"]
+ [1, null, "2"] | [1, null, "2"] | [1, null, "2"] | [[1,
null, "2"]] | [[1, null, "2"]]
+ {"a": 1, "b": [2]} | {"a": 1, "b": [2]} | {"a": 1, "b": [2]} |
[{"a": 1, "b": [2]}] | [{"a": 1, "b": [2]}]
+(6 rows)

Just a suggestion if we can have column aliases for better
understanding like we are doing for other test cases in the same
patch?

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: SQL/JSON: functions

[Himanshu Upadhyaya]

On Thu, Dec 9, 2021 at 7:34 PM Himanshu Upadhyaya
 wrote:
> 3)
> Is not that result of the two below queries should match because both are 
> trying to retrieve the information from the JSON object.
>
> postgres=# SELECT JSON_OBJECT('track' VALUE '{
> "segments": [
>   {
> "location":   [ 47.763, 13.4034 ],
> "start time": "2018-10-14 10:05:14",
> "HR": 73
>   },
>   {
> "location":   [ 47.706, 13.2635 ],
> "start time": "2018-10-14 101:39:21",
> "HR": 135
>   }
> ]
>   }
> }')->'track'->'segments';
>  ?column?
> --
>
> (1 row)
>
> postgres=# select '{
>   "track": {
> "segments": [
>   {
> "location":   [ 47.763, 13.4034 ],
> "start time": "2018-10-14 10:05:14",
> "HR": 73
>   },
>   {
> "location":   [ 47.706, 13.2635 ],
> "start time": "2018-10-14 10:39:21",
> "HR": 135
>   }
> ]
>   }
> }'::jsonb->'track'->'segments';
>  
> ?column?
> ---
>  [{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14 
> 10:05:14"}, {"HR": 135, "location": [47.706, 13.2635], "start time": 
> "2018-10-14 10:39:21"}]
> (1 row)
>
just wanted to check your opinion on the above, is this an expected behaviour?

> Few comments For 0002-SQL-JSON-constructors-v59.patch:
Also, any thoughts on this?

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: SQL/JSON: functions

[Himanshu Upadhyaya]

On Thu, Dec 16, 2021 at 3:06 AM Andrew Dunstan  wrote:
>
>
> On 12/9/21 09:04, Himanshu Upadhyaya wrote:
> >
> >
> >
> > 4)
> > Are we intentionally allowing numeric keys in JSON_OBJECT but somehow
> > these are not allowed in ORACLE?
> > ‘postgres[151876]=#’select JSON_OBJECT( 3+1:2, 2+2:1);
> > json_object
> > 
> >  {"4" : 2, "4" : 1}
> > (1 row)
> >
> > In ORACLE we are getting error("ORA-00932: inconsistent datatypes:
> > expected CHAR got NUMBER") which seems to be more reasonable.
> > "ORA-00932: inconsistent datatypes: expected CHAR got NUMBER"
> >
> > Postgres is also dis-allowing below then why allow numeric keys in
> > JSON_OBJECT?
> > ‘postgres[151876]=#’select '{
> >   "track": {
> > "segments": [
> >   {
> > "location":   [ 47.763, 13.4034 ],
> > "start time": "2018-10-14 10:05:14",
> > "HR": 73
> >   },
> >   {
> > "location":   [ 47.706, 13.2635 ],
> > "start time": "2018-10-14 10:39:21",
> > 3: 135
> >   }
> > ]
> >   }
> > }'::jsonb;
> > ERROR:  22P02: invalid input syntax for type json
> > LINE 1: select '{
> >^
> > DETAIL:  Expected string, but found "3".
> > CONTEXT:  JSON data, line 12: 3...
> > LOCATION:  json_ereport_error, jsonfuncs.c:621
> >
> > Also, JSON_OBJECTAGG is failing if we have any numeric key, however,
> > the message is not very appropriate.
> > SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
> > FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
> > (5,5)) kv(k, v);
> > ERROR:  22P02: invalid input syntax for type integer: "no"
> > LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
> >   ^
> > LOCATION:  pg_strtoint32, numutils.c:320
> >
> >
> >
>
> The literal above is simply not legal json, so the json parser is going
> to reject it outright. However it is quite reasonable for JSON
> constructors to convert non-string key values to strings. Otherwise we'd
> be rejecting not just numbers but for example dates as key values. c.f.
> json_build_object(), the documentation for which says "Key arguments are
> coerced to text."
>
Yes Agree on this, but just thinking if we can differentiate dates and
numeric keys to have consistent behaviour and simply reject if we have
numeric keys(to match it with the behaviour of JSON parser) because
JSON with numeric keys is actually not a valid JSON.

SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
(5,5)) kv(k, v);
ERROR:  22P02: invalid input syntax for type integer: "no"
LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
  ^
LOCATION:  pg_strtoint32, numutils.c:320

Above call to JSON_OBJECTAGG is failing because we have the numeric
key, is not that it also needs to follow the same context  of
converting key argument to text? or both(JSON_OBJECTAGG  and
JSON_OBJECT) should not allow numeric keys in the JSON object and
allow date (if that is the only use case)?

Thoughts?
-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: SQL/JSON: functions

[Himanshu Upadhyaya]

  Oid typid =
exprType(lfirst(lc));
+
+   if (is_jsonb ?
+   !to_jsonb_is_immutable(typid) :
+   !to_json_is_immutable(typid))
+   return true;
+   }
+
+   /* Check all subnodes */
+   }
can have ctor as const pointer?

2)
+typedef struct JsonFormat
+{
+   NodeTag type;
+   JsonFormatType format;  /* format type */
+   JsonEncoding encoding;  /* JSON encoding */
+   int location;   /* token location,
or -1 if unknown */
+} JsonFormat;

I think it will be good if we can have a JsonformatType(defined in patch
0001-Common-SQL-JSON-clauses-v59.patch) member named as
format_type or formatType instead of format?
There are places in the patch where we access it as "if (format->format ==
JS_FORMAT_DEFAULT)". "format->format" looks little difficult to understand.
"format->format_type == JS_FORMAT_DEFAULT" will be easy to follow.

3)
+   if (have_jsonb)
+   {
+   returning->typid = JSONBOID;
+   returning->format->format = JS_FORMAT_JSONB;
+   }
+   else if (have_json)
+   {
+   returning->typid = JSONOID;
+   returning->format->format = JS_FORMAT_JSON;
+   }
+   else
+   {
+   /* XXX TEXT is default by the standard, but we
return JSON */
+   returning->typid = JSONOID;
+   returning->format->format = JS_FORMAT_JSON;
+   }

why we need a separate "else if (have_json)" statement in the below code,
"else" is also doing the same thing?

4)
-test: json jsonb json_encoding jsonpath jsonpath_encoding jsonb_jsonpath
+test: json jsonb json_encoding jsonpath jsonpath_encoding jsonb_jsonpath
sqljson

can we rename sqljson sql test file to json_constructor?

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: SQL/JSON: functions

[Himanshu Upadhyaya]

On Wed, Dec 1, 2021 at 7:56 PM Andrew Dunstan  wrote:

>
> On 12/1/21 06:13, Himanshu Upadhyaya wrote:
> > Hi Andrew,
> >
> > The latest version (v59) is not applying on head.
> > Could you please help to rebase?
> >
> >
>
> (Please don't top-post on PostgreSQL lists)
>
> Sure, I will take care of that in the future.

The patches apply for me and for the cfbot:
> <http://cfbot.cputube.org/patch_35_2901.log>. I'm not sure what's not
> working for you. I apply them using "patch -p 1 < $patchfile"
>
> Mistakenly I was using git apply, sorry about that. It's working fine with
"patch -p 1 < $patchfile".

-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

Re: SQL/JSON: functions

[Himanshu Upadhyaya]

Hi Andrew,

The latest version (v59) is not applying on head.
Could you please help to rebase?

Thanks,
Himanshu

On Thu, Sep 16, 2021 at 8:23 PM Andrew Dunstan  wrote:

>
> On 9/14/21 8:55 AM, Andrew Dunstan wrote:
> > On 9/2/21 2:50 PM, Andrew Dunstan wrote:
> >> On 5/18/21 3:22 PM, Andrew Dunstan wrote:
> >>> On 5/8/21 2:21 PM, Andrew Dunstan wrote:
>  On 4/28/21 5:55 PM, Andrew Dunstan wrote:
> > On Fri, Mar 26, 2021 at 9:14 PM Nikita Glukhov
> > mailto:n.glu...@postgrespro.ru>> wrote:
> >
> > Attached 54th version of the patches rebased onto current master.
> >
> > On 27.03.2021 01:30, Andrew Dunstan wrote:
> >> Specifically, patch 4 (SQL-JSON-query-functions) fails with
> this when
> >> built with LLVM:
> >>
> >>
> >> There is also a bug that results in a warning in gram.y, but
> fixing it
> >> doesn't affect this issue. Nikita, please look into this ASAP.
> > LLVM issues and gram.y are fixed.
> >
> >
> >
> >
> > It's apparently bitrotted again. See
> >  > >
> >
> >
>  This set should remove the bitrot.
> 
> 
> >>> Rebased for removal of serial schedule
> >>>
> >>>
> >> rebased on master and incorporating fixes from Erik Rijkers
> >>
> >>
> > rebased to remove bitrot from the removal of the  Value node type.
> >
> >
>
> Rebased, and fixed a bug (which I had faithfully replicated above) in
> the APP_JUMB code, as reported by Erik Rijkers.
>
>
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
>

Should we support new definition for Identity column : GENERATED BY DEFAULT ON NULL?

[Himanshu Upadhyaya]

Hi,

Trying to insert NULL value to the Identity column defined by "GENERATED BY
DEFAULT" is disallowed, but there can be use cases where the user would
like to have an identity column where manual NULL insertion is required(and
it should not error-out by Postgres).

How about having a new type for the Identity column as "GENERATED BY
DEFAULT ON NULL", which will allow manual NULL insertion and internally
NULL value will be replaced by Sequence NextValue?

ORACLE is supporting this feature by having a similar Identity column type
as below:
===
SQL> CREATE TABLE itest1 (id1 INTEGER GENERATED BY DEFAULT ON NULL
AS IDENTITY, id2 INTEGER);

Table created.

SQL> INSERT INTO itest1 VALUES (NULL, 10);--Supported with GENERATED BY
DEFAULT ON NULL

1 row created.

SQL> INSERT INTO itest1 VALUES (1,30);

1 row created.

SQL> INSERT INTO itest1 (id2) VALUES (20);

1 row created.

SQL> SELECT * FROM itest1;

   ID1ID2
-- --
 1 10
 1 30
 2 20


I think it is good to have support for GENERATED BY DEFAULT ON NULL in
Postgres.

Thoughts?

Thanks,
Himanshu

Re: inconsistent behavior with "GENERATED BY DEFAULT AS IDENTITY"

[Himanshu Upadhyaya]

ok, understood.

Thanks Tom.

Regards,
Himanshu

On Sun, Aug 29, 2021 at 7:10 PM Tom Lane  wrote:

> Himanshu Upadhyaya  writes:
> > IMHO below query should replace "NULL" value for ID column with the
> > GENERATED IDENTITY value (should insert 1,10 for ID and ID1 respectively
> in
> > below's example), similar to what we expect when we have DEFAULT
> constraint
> > on the column.
>
> Why?  Ordinary DEFAULT clauses do not act that way; if you specify NULL
> (or any other value) that is what you get.  If you want the default
> value, you can omit the column, or write DEFAULT.
>
> > Any reason for disallowing NULL insertion?
>
> Consistency and standards compliance.
>
> regards, tom lane
>

inconsistent behavior with "GENERATED BY DEFAULT AS IDENTITY"

[Himanshu Upadhyaya]

Hi,

It seems we have inconsistent behavior with the implementation of
"GENERATED BY DEFAULT AS IDENTITY" constraint on a table column.
Here we are not allowing(internally not replacing NULL with IDENTITY
DEFAULT) the "NULL" insertion into the table column.

postgres=# CREATE TABLE TEST_TBL_1(ID INTEGER  GENERATED BY DEFAULT AS
IDENTITY ,ID1 INTEGER);
CREATE TABLE
postgres=# insert into TEST_TBL_1 values  (NULL, 10);
ERROR:  null value in column "id" of relation "test_tbl_1" violates
not-null constraint
DETAIL:  Failing row contains (null, 10).
postgres=# insert into TEST_TBL_1(id1) values  ( 10);
INSERT 0 1


However this is allowed on normal default column:
postgres=# create table TEST_TBL_2 (ID INTEGER  DEFAULT 10 ,ID1 INTEGER);
CREATE TABLE
postgres=# insert into TEST_TBL_2 values  (NULL, 10);
INSERT 0 1
postgres=# insert into TEST_TBL_2 (id1) values  (20);
INSERT 0 1


if I understand it correctly, the use-case for supporting "GENERATED BY
DEFAULT AS IDENTITY" is to have an inbuilt sequence generated DEFAULT value
for a column.

IMHO below query should replace "NULL" value for ID column with the
GENERATED IDENTITY value (should insert 1,10 for ID and ID1 respectively in
below's example), similar to what we expect when we have DEFAULT constraint
on the column.

insert into TEST_TBL_1 values  (NULL, 10);

TO Support the above query ORACLE is having "GENERATED BY DEFAULT ON NULL
AS IDENTITY" syntax. We can also think on similar lines and have similar
implementation
or allow it under "GENERATED BY DEFAULT AS IDENTITY" itself.

Any reason for disallowing NULL insertion?

Thoughts?

Thanks,
Himanshu

Re: Support reset of Shared objects statistics in "pg_stat_reset" function

[Himanshu Upadhyaya]

Hi Sadhu,

Patch working as expected with shared tables, just one Minor comment on the
patch.
+   if (!dbentry)
+   return;
+
+   /*
+* We simply throw away all the shared table entries by recreating
new
+* hash table for them.
+*/
+   if (dbentry->tables != NULL)
+   hash_destroy(dbentry->tables);
+   if (dbentry->functions != NULL)
+   hash_destroy(dbentry->functions);
+
+   dbentry->tables = NULL;
+   dbentry->functions = NULL;
+
+   /*
+* This creates empty hash tables for tables and functions.
+*/
+   reset_dbentry_counters(dbentry);

We already have the above code for non-shared tables, can we restrict
duplicate code?
one solution I can think of, if we can have a list with two elements and
iterate each element with
these common steps?

Thanks,
Himanshu

On Fri, Aug 6, 2021 at 5:40 PM Himanshu Upadhyaya <
upadhyaya.himan...@gmail.com> wrote:

> Hi Sadhu,
>
>
> > The call to “pg_stat_reset“ does reset all the statistics data for
> > tables belonging to the current database but does not take care of
> > shared tables e.g pg_attribute.
>
> pg_attribute is not a shared catalog, is the mentioned scenario is also
> applicable for few others tables?
>
> I have just tried it with-out your patch:
>
> postgres=# SELECT * FROM pg_statio_all_tables  where relid=1249;
>  relid | schemaname |   relname| heap_blks_read | heap_blks_hit |
> idx_blks_read | idx_blks_hit | toast_blks_read | toast_blks_hit |
> tidx_blks_read | tidx_blks_hit
>
> ---++--++---+---+--+-+++---
>   1249 | pg_catalog | pg_attribute | 29 |   522 |
> 8 |  673 | ||
>  |
> (1 row)
>
> postgres=# select pg_stat_reset();
>  pg_stat_reset
> ---
>
> (1 row)
>
> postgres=# SELECT * FROM pg_statio_all_tables  where relid=1249;
>  relid | schemaname |   relname| heap_blks_read | heap_blks_hit |
> idx_blks_read | idx_blks_hit | toast_blks_read | toast_blks_hit |
> tidx_blks_read | tidx_blks_hit
>
> ---++--++---+---+--+-+++---
>   1249 | pg_catalog | pg_attribute |  0 | 0 |
> 0 |0 | ||
>  |
>
>
> We are able to reset the stats of pg_attibute without your patch.
>
> Thanks,
> Himanshu
>
> On Fri, Aug 6, 2021 at 1:56 PM Sadhuprasad Patro 
> wrote:
>
>> Hi,
>>
>> The call to “pg_stat_reset“ does reset all the statistics data for
>> tables belonging to the current database but does not take care of
>> shared tables e.g pg_attribute. Similarly to reset the statistics at
>> table level, the call to “pg_stat_reset_single_table_counters“ does
>> not take care of shared tables.
>>
>> When we reset all the statistics using the call “pg_stat_reset”, the
>> postgres process internally makes calls to “
>> pgstat_recv_resetcounter“, which resets the statistics of all the
>> tables of the current database. But not resetting the statistics of
>> shared objects using database ID as 'InvalidOid'.
>>
>> The same fix is made in the internal function
>> “pgstat_recv_resetsinglecounter“ to reset the statistics for the
>> shared table for the call "pg_stat_reset_single_table_counters".
>>
>> --
>> thank u
>> SADHU PRASAD
>> EnterpriseDB: http://www.enterprisedb.com
>>
>

Re: Support reset of Shared objects statistics in "pg_stat_reset" function

[Himanshu Upadhyaya]

Hi Sadhu,


> The call to “pg_stat_reset“ does reset all the statistics data for
> tables belonging to the current database but does not take care of
> shared tables e.g pg_attribute.

pg_attribute is not a shared catalog, is the mentioned scenario is also
applicable for few others tables?

I have just tried it with-out your patch:

postgres=# SELECT * FROM pg_statio_all_tables  where relid=1249;
 relid | schemaname |   relname| heap_blks_read | heap_blks_hit |
idx_blks_read | idx_blks_hit | toast_blks_read | toast_blks_hit |
tidx_blks_read | tidx_blks_hit
---++--++---+---+--+-+++---
  1249 | pg_catalog | pg_attribute | 29 |   522 |
  8 |  673 | ||
 |
(1 row)

postgres=# select pg_stat_reset();
 pg_stat_reset
---

(1 row)

postgres=# SELECT * FROM pg_statio_all_tables  where relid=1249;
 relid | schemaname |   relname| heap_blks_read | heap_blks_hit |
idx_blks_read | idx_blks_hit | toast_blks_read | toast_blks_hit |
tidx_blks_read | tidx_blks_hit
---++--++---+---+--+-+++---
  1249 | pg_catalog | pg_attribute |  0 | 0 |
  0 |0 | ||
 |


We are able to reset the stats of pg_attibute without your patch.

Thanks,
Himanshu

On Fri, Aug 6, 2021 at 1:56 PM Sadhuprasad Patro  wrote:

> Hi,
>
> The call to “pg_stat_reset“ does reset all the statistics data for
> tables belonging to the current database but does not take care of
> shared tables e.g pg_attribute. Similarly to reset the statistics at
> table level, the call to “pg_stat_reset_single_table_counters“ does
> not take care of shared tables.
>
> When we reset all the statistics using the call “pg_stat_reset”, the
> postgres process internally makes calls to “
> pgstat_recv_resetcounter“, which resets the statistics of all the
> tables of the current database. But not resetting the statistics of
> shared objects using database ID as 'InvalidOid'.
>
> The same fix is made in the internal function
> “pgstat_recv_resetsinglecounter“ to reset the statistics for the
> shared table for the call "pg_stat_reset_single_table_counters".
>
> --
> thank u
> SADHU PRASAD
> EnterpriseDB: http://www.enterprisedb.com
>

Re: [PATCH] PREPARE TRANSACTION unexpected behavior with TEMP TABLE

[Himanshu Upadhyaya]

Hi Robert,

Thanks for sharing your thoughts.
The purpose of this FIX is to mainly focus on getting consistent behavior
with PREPARE TRANSACTION. With the case that I had mentioned
previously, my expectation was either both PREPARE TRANSACTION should fail
or both should succeed but here second same "PREPARE TRANSACTION" was
successful however first one was failing with an error, which is kind of
weird to me.


I have also tried to reproduce the behavior.

[session:1]
postgres=# create temp table fullname (first text, last text);
CREATE TABLE

[session:2]
postgres=# select oid::regclass from pg_class where relname = 'fullname';
oid

 pg_temp_3.fullname

postgres=# BEGIN;
create function longname1(pg_temp_3.fullname) returns text language sql
as $$select $1.first || ' ' || $1.last$$;
BEGIN
CREATE FUNCTION
postgres=*# prepare transaction 'mytran2';
ERROR:  cannot PREPARE a transaction that has operated on temporary objects
postgres=# BEGIN;
create function longname1(pg_temp_3.fullname) returns text language sql
as $$select $1.first || ' ' || $1.last$$;
BEGIN
CREATE FUNCTION

[session:1]
postgres=# \q// no problem in exiting

[session:2]

postgres=*# prepare transaction 'mytran2';
PREPARE TRANSACTION
postgres=# \df
ERROR:  cache lookup failed for type 16429

looking at the comment in the code [session:1] should hang while exiting
but
I don't see a problem here, you have already explained that in your reply.
Even then I feel that behavior should be consistent when we mix temporary
objects in PREPARE TRANSACTION.

The comments in
> PrepareTransaction() justify this prohibition by saying that "Having
> the prepared xact hold locks on another backend's temp table seems
> a bad idea --- for instance it would prevent the backend from exiting.
> There are other problems too, such as how to clean up the source
> backend's local buffers and ON COMMIT state if the prepared xact
> includes a DROP of a temp table."
>
I can see from the above experiment that there is no problem with the
lock in the above case but not sure if there is any issue with "clean up
the source backend's local buffers", if not then we don't even need this
ERROR (ERROR:  cannot PREPARE a transaction that has operated
on temporary objects) in PREPARE TRANSACTION?

To really fix this, you'd need CREATE FUNCTION to take a lock on the
> containing namespace, whether permanent or temporary. You'd also need
> every other CREATE statement that creates a schema-qualified object to
> do the same thing. Maybe that's a good idea, but we've been reluctant
> to go that far in the past due to performance consequences, and it's
> not clear whether any of those problems are related to the issue that
> prompted you to submit the patch.

Yes, the purpose of this patch is to actually have a valid value in
XACT_FLAGS_ACCESSEDTEMPNAMESPACE, having said that it should
always be true if we access temporary object else false.
Even if we do changes to have lock in case of "CREATE FUNCTION", we
also need to have this FIX in place so that "PREPARE TRANSACTION"
mixed with TEMPORARY OBJECT will always be restricted and will not
cause any hang issue(which we will start observing once we implement
these "CREATE STATEMENT" changes) as mentioned in the comment
in the PrepareTransaction().

Just thinking if it's acceptable to FIX this and make it consistent by
properly
setting XACT_FLAGS_ACCESSEDTEMPNAMESPACE, so that it should always
fail if we access the temporary object, I also agree here that it will not
actually cause
any issue because of xact lock but then from user perspective it seems
weird
when the same PREPARE TRANSACTION is working second time onwards, thoughts?

Thanks,
Himanshu


On Thu, Apr 8, 2021 at 7:43 PM Robert Haas  wrote:

> On Wed, Apr 7, 2021 at 12:19 PM Himanshu Upadhyaya
>  wrote:
> > Please find attached the V2 patch.
>
> Hi,
>
> This patch is essentially taking the position that calling
> load_typcache_tupdesc before using that tupdesc for anything is
> required for correctness. I'm not sure whether that's a good
> architectural decision: to me, it looks like whoever wrote this code
> originally - I think it was Tom - had the idea that it would be OK to
> skip calling that function whenever we already have the value.
> Changing that has some small performance cost, and it also just looks
> kind of weird, because you don't expect a function called
> load_typcache_tupdesc() to have the side effect of preventing some
> kind of bad thing from happening. You just expect it to be loading
> stuff. The comments in this code are not exactly stellar as things
> stand, but the patch also doesn't update them in a meaningful way.
> Sure, it corrects a few comments that would be flat-out wrong
> otherwise, but it doesn't add any kind of explanation that would

Re: [PATCH] PREPARE TRANSACTION unexpected behavior with TEMP TABLE

[Himanshu Upadhyaya]

Hi Vignesh,
Thanks for sharing the review comments. Please find my response below.

> 1) We can drop the table after this test.
>
Done.

> 2) +-- Test for accessing Temporary table
> +-- in prepare transaction.
> can be changed to
> -- Test for accessing cached temporary table in a prepared transaction.
>
Comment is now modified as above.

> 3) +-- These cases must fail and generate errors about Temporary objects.
> can be changed to
> -- These cases should fail with cannot access temporary object error.
>
The error is not about accessing the temporary object rather it's about
disallowing create transaction as it is referring to the temporary objects.
I have changed it with the exact error we get in those cases.

Please find attached the V2 patch.

Thanks,
Himanshu

On Wed, Apr 7, 2021 at 4:11 PM vignesh C  wrote:

> On Tue, Apr 6, 2021 at 8:18 PM Himanshu Upadhyaya
>  wrote:
> >
> > Hi,
> >
> > While working on one of the issue, I have noticed below unexpected
> behavior with "PREPARE TRANSACTION".
> >
> > We are getting this unexpected behavior with PREPARE TRANSACTION when it
> is mixed with Temporary Objects. Please consider the below setup and SQL
> block.
> >
> > set max_prepared_transactions to 1 (or any non zero value), this is to
> enable the “prepare transaction”.
> >
> > Now please try to run the below set of statements.
> > [BLOCK-1:]
> > postgres=# create temp table fullname (first text, last text);
> > CREATE TABLE
> > postgres=# BEGIN;
> > BEGIN
> > postgres=*# create function longname(fullname) returns text language sql
> > postgres-*# as $$select $1.first || ' ' || $1.last$$;
> > CREATE FUNCTION
> > postgres=*# prepare transaction 'mytran';
> > ERROR:  cannot PREPARE a transaction that has operated on temporary
> objects
> >
> > Above error is expected.
> >
> > The problem is if we again try to create the same function in the
> “PREPARE TRANSACTION” as below.
> >
> > [BLOCK-2:]
> > postgres=# BEGIN;
> > BEGIN
> > postgres=*# create function longname(fullname) returns text language sql
> > as $$select $1.first || ' ' || $1.last$$;
> > CREATE FUNCTION
> > postgres=*# PREPARE transaction 'mytran';
> > PREPARE TRANSACTION
> >
> > Now, this time we succeed and not getting the above error (”ERROR:
> cannot PREPARE a transaction that has operated on temporary objects), like
> the way we were getting with BLOCK-1
> >
> > This is happening because we set MyXactFlags in relation_open function
> call, and here relation_open is getting called from load_typcache_tupdesc,
> but in the second run of “create function…” in the above #2 block will not
> call load_typcache_tupdesc because of the below condition(typentry->tupDesc
> == NULL) in  lookup_type_cache().
> >
> > /*
> >  * If it's a composite type (row type), get tupdesc if requested
> >  */
> > if ((flags & TYPECACHE_TUPDESC) &&
> > typentry->tupDesc == NULL &&
> > typentry->typtype == TYPTYPE_COMPOSITE)
> > {
> > load_typcache_tupdesc(typentry);
> > }
> >
> > We set typentry->tupDesc to non NULL(and populates it with proper tuple
> descriptor in the cache) value during our first call to “create function…”
> in BLOCK-1.
> > We have logic in file xact.c::PrepareTransaction() to simply error out
> if we have accessed any temporary object in the current transaction but
> because of the above-described issue of not setting
> XACT_FLAGS_ACCESSEDTEMPNAMESPACE in MyXactFlags second run of “create
> function..” Works and PREPARE TRANSACTION succeeds(but it should fail).
> >
> > Please find attached the proposed patch to FIX this issue.
>
> I was able to reproduce the issue with your patch and your patch fixes
> the issue.
>
> Few comments:
> 1) We can drop the table after this test.
> +CREATE TEMP TABLE temp_tbl (first TEXT, last TEXT);
> +BEGIN;
> +CREATE FUNCTION longname(temp_tbl) RETURNS TEXT LANGUAGE SQL
> +AS $$SELECT $1.first || ' ' || $1.last$$;
> +PREPARE TRANSACTION 'temp_tbl_access';
> +
> +BEGIN;
> +CREATE FUNCTION longname(temp_tbl) RETURNS TEXT LANGUAGE SQL
> +AS $$SELECT $1.first || ' ' || $1.last$$;
> +PREPARE TRANSACTION 'temp_tbl_access';
>
> 2) +-- Test for accessing Temporary table
> +-- in prepare transaction.
> can be changed to
> -- Test for accessing cached temporary table in a prepared transaction.
>
> 3) +-- These cases must fail and generate errors about Temporary objects.
> can be changed to
> -- These cases should

[PATCH] PREPARE TRANSACTION unexpected behavior with TEMP TABLE

[Himanshu Upadhyaya]

Hi,

While working on one of the issue, I have noticed below unexpected behavior
with "PREPARE TRANSACTION".

We are getting this unexpected behavior with PREPARE TRANSACTION when it is
mixed with Temporary Objects. Please consider the below setup and SQL block.

set max_prepared_transactions to 1 (or any non zero value), this is to
enable the “prepare transaction”.

Now please try to run the below set of statements.
[BLOCK-1:]
postgres=# create temp table fullname (first text, last text);
CREATE TABLE
postgres=# BEGIN;
BEGIN
postgres=*# create function longname(fullname) returns text language sql
postgres-*# as $$select $1.first || ' ' || $1.last$$;
CREATE FUNCTION
postgres=*# prepare transaction 'mytran';
ERROR:  cannot PREPARE a transaction that has operated on temporary objects

Above error is expected.

The problem is if we again try to create the same function in the “PREPARE
TRANSACTION” as below.

[BLOCK-2:]
postgres=# BEGIN;
BEGIN
postgres=*# create function longname(fullname) returns text language sql
as $$select $1.first || ' ' || $1.last$$;
CREATE FUNCTION
postgres=*# PREPARE transaction 'mytran';
PREPARE TRANSACTION

Now, this time we succeed and not getting the above error (”ERROR:  cannot
PREPARE a transaction that has operated on temporary objects), like the way
we were getting with BLOCK-1

This is happening because we set MyXactFlags in relation_open function
call, and here relation_open is getting called from load_typcache_tupdesc,
but in the second run of “create function…” in the above #2 block will not
call load_typcache_tupdesc because of the below condition(typentry->tupDesc
== NULL) in  lookup_type_cache().

/*
 * If it's a composite type (row type), get tupdesc if requested
 */
if ((flags & TYPECACHE_TUPDESC) &&
typentry->tupDesc == NULL &&
typentry->typtype == TYPTYPE_COMPOSITE)
{
load_typcache_tupdesc(typentry);
}

We set typentry->tupDesc to non NULL(and populates it with proper tuple
descriptor in the cache) value during our first call to “create function…”
in BLOCK-1.
We have logic in file xact.c::PrepareTransaction() to simply error out if
we have accessed any temporary object in the current transaction but
because of the above-described issue of not setting
XACT_FLAGS_ACCESSEDTEMPNAMESPACE in MyXactFlags second run of “create
function..” Works and PREPARE TRANSACTION succeeds(but it should fail).

Please find attached the proposed patch to FIX this issue.

Thoughts?

Thanks,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From 6e2a6bdfb77870213d051f31b2b6683ccea45ab2 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Mon, 5 Apr 2021 21:04:56 +0530
Subject: [PATCH v1] PREPARE TRANSACTION must fail when mixed with temporary
 object.

---
 src/backend/utils/cache/typcache.c | 30 --
 src/test/regress/expected/temp.out | 14 ++
 src/test/regress/sql/temp.sql  | 14 ++
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 4915ef5934..36fac6150d 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -813,7 +813,6 @@ lookup_type_cache(Oid type_id, int flags)
 	 * If it's a composite type (row type), get tupdesc if requested
 	 */
 	if ((flags & TYPECACHE_TUPDESC) &&
-		typentry->tupDesc == NULL &&
 		typentry->typtype == TYPTYPE_COMPOSITE)
 	{
 		load_typcache_tupdesc(typentry);
@@ -881,21 +880,25 @@ load_typcache_tupdesc(TypeCacheEntry *typentry)
 
 	/*
 	 * Link to the tupdesc and increment its refcount (we assert it's a
-	 * refcounted descriptor).  We don't use IncrTupleDescRefCount() for this,
-	 * because the reference mustn't be entered in the current resource owner;
+	 * refcounted descriptor), do this only if it was not populated previously.
+	 * We don't use IncrTupleDescRefCount() for this, because the reference
+	 * mustn't be entered in the current resource owner;
 	 * it can outlive the current query.
 	 */
-	typentry->tupDesc = RelationGetDescr(rel);
+	if (typentry->tupDesc == NULL)
+	{
+		typentry->tupDesc = RelationGetDescr(rel);
 
-	Assert(typentry->tupDesc->tdrefcount > 0);
-	typentry->tupDesc->tdrefcount++;
+		Assert(typentry->tupDesc->tdrefcount > 0);
+		typentry->tupDesc->tdrefcount++;
 
-	/*
-	 * In future, we could take some pains to not change tupDesc_identifier if
-	 * the tupdesc didn't really change; but for now it's not worth it.
-	 */
-	typentry->tupDesc_identifier = ++tupledesc_id_counter;
+		/*
+		 * In future, we could take some pains to not change tupDesc_identifier if
+		 * the tupdesc didn't really change; but for now it's not worth it.
+		 */
+		typentry->tupDesc_identifier = ++tupledesc_id_counter;
 
+	

Inaccurate comment, for family-1 polymorphic UNKNOWN type arguments

[Himanshu Upadhyaya]

Hi Hackers,

I observed that we have inaccurate comment in
enforce_generic_type_consistency.

 if (!OidIsValid(elem_typeid))
{
if (allow_poly)
{
elem_typeid = ANYELEMENTOID;
array_typeid = ANYARRAYOID;
range_typeid = ANYRANGEOID;
}
else
{
/*
 * Only way to get here is if all the
polymorphic args have
 * UNKNOWN inputs
 */
*ereport(ERROR, *
  ...
 }

}
We reach the error condition even if there is any "anycompatible" parameter
is present (and that is of some known type say int32).
I think developer intend to report error if "we have all the family-1
polymorphic arguments as UNKNOWN".

Thoughts?

Please find attached the patch to fix this typo.

Thanks,
Himanshu
EnterpriseDB: http://www.enterprisedb.com
From 361b8aac87fb3d4e4c968555459f1cedec489440 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya 
Date: Mon, 7 Sep 2020 16:48:22 +0530
Subject: [PATCH v1] FIX- for incorrect comment for UNKNOWN input type for
 polymorphic arguments

---
 src/backend/parser/parse_coerce.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 1b11cf731c..36d6483c24 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -2155,7 +2155,7 @@ enforce_generic_type_consistency(const Oid *actual_arg_types,
 			else
 			{
 /*
- * Only way to get here is if all the polymorphic args have
+ * Only way to get here is if all the family-1 polymorphic args have
  * UNKNOWN inputs
  */
 ereport(ERROR,
-- 
2.25.1