Re: CHECK Constraint Deferrable
On Thu, 14 Sept 2023 at 15:33, Himanshu Upadhyaya wrote: > > > > 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 I think we should be able to defer one constraint like in the case of foreign key constraint: create table t1(c1 int primary key); insert into t1 values(10); create table t2(c1 int primary key); insert into t2 values(10); create table t3(c1 int, c2 int references t1(c1) deferrable, c3 int references t2(c1) deferrable); -- Set only one constraint as deferred begin; set CONSTRAINTS t3_c2_fkey deferred; -- c2 column constraint is deferred, we need not set all constraints deferred in this case, insert was successful postgres=*# insert into t3 values(1,11,10); INSERT 0 1 -- Throws error for the constraint that is not deferred postgres=*# insert into t3 values(1,10,11); ERROR: insert or update on table "t3" violates foreign key constraint "t3_c3_fkey" DETAIL: Key (c3)=(11) is not present in table "t2". Thoughts? Regards, Vignesh
Re: Bug fix for psql's meta-command \ev
On Fri, Sep 15, 2023 at 11:37:46AM +0900, Ryoga Yoshida wrote: > I think this is a bug in psql's \ev meta-command. Even when \ev fails, it > should not leave the garbage string in psql's query buffer and the following > query should be completed successfully. Right. Good catch. Will look at that a bit more to see if the resets are correctly placed, particularly in light of \ef. -- Michael signature.asc Description: PGP signature
Re: Have better wording for snapshot file reading failure
On Thu, Sep 14, 2023 at 05:33:35PM -0700, Andres Freund wrote:
> I'd probably just go for something like "snapshot \"%s\" does not exist",
> similar to what we report for unknown tables etc. Arguably changing the
> errcode to ERRCODE_UNDEFINED_OBJECT would make this more precise?
Good points. Updated as suggested in v2 attached.
--
Michael
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index b20440ee21..4a3613d15f 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1390,9 +1390,21 @@ ImportSnapshot(const char *idstr)
f = AllocateFile(path, PG_BINARY_R);
if (!f)
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("invalid snapshot identifier: \"%s\"", idstr)));
+ {
+ /*
+ * If file is missing while identifier has a correct format, avoid
+ * system errors.
+ */
+ if (errno == ENOENT)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("snapshot \"%s\" does not exist", idstr)));
+ else
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not open file \"%s\" for reading: %m",
+ path)));
+ }
/* get the size of the file so that we know how much memory we need */
if (fstat(fileno(f), _buf))
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 428c9edcc6..7717967a9f 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -1148,6 +1148,17 @@ SELECT * FROM trans_abc ORDER BY 1;
(3 rows)
DROP TABLE trans_abc;
+-- TRANSACTION SNAPSHOT
+-- Incorrect identifier.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'Incorrect Identifier';
+ERROR: invalid snapshot identifier: "Incorrect Identifier"
+ROLLBACK;
+-- Correct identifier, missing file.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'FFF-FFF-F';
+ERROR: snapshot "FFF-FFF-F" does not exist
+ROLLBACK;
-- Test for successful cleanup of an aborted transaction at session exit.
-- THIS MUST BE THE LAST TEST IN THIS FILE.
begin;
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index 75ffe929d4..db2535c787 100644
--- a/src/test/regress/sql/transactions.sql
+++ b/src/test/regress/sql/transactions.sql
@@ -613,6 +613,15 @@ SELECT * FROM trans_abc ORDER BY 1;
DROP TABLE trans_abc;
+-- TRANSACTION SNAPSHOT
+-- Incorrect identifier.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'Incorrect Identifier';
+ROLLBACK;
+-- Correct identifier, missing file.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'FFF-FFF-F';
+ROLLBACK;
-- Test for successful cleanup of an aborted transaction at session exit.
-- THIS MUST BE THE LAST TEST IN THIS FILE.
signature.asc
Description: PGP signature
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Peter, Thank you for reviewing! New patch is available in [1]. > 1. > Configure the servers for log shipping. (You do not need to run > pg_backup_start() and > pg_backup_stop() > or take a file system backup as the standbys are still synchronized > - with the primary.) Replication slots are not copied and must > - be recreated. > + with the primary.) Only logical slots on the primary are copied to the > + new standby, and other other slots on the old standby must be > recreated > + as they are not copied. > > > IMO this text still needs some minor changes like shown below, Anyway, > there is a typo: /other other/ > > SUGGESTION > Only logical slots on the primary are copied to the new standby, but > other slots on the old standby are not copied so must be recreated > manually. > Fixed. > == > src/bin/pg_upgrade/server.c > > 2. > + * > + * Use max_slot_wal_keep_size as -1 to prevent the WAL removal by the > + * checkpointer process. If WALs required by logical replication slots are > + * removed, the slots are unusable. The setting ensures that such WAL > + * records have remained so that invalidation of slots would be avoided > + * during the upgrade. > > The comment already explained the reason for the setting is to prevent > removing the needed WAL records, so I felt there is no need for the > last sentence to repeat the same information. > > BEFORE > The setting ensures that such WAL records have remained so that > invalidation of slots would be avoided during the upgrade. > > SUGGESTION > This setting prevents the invalidation of slots during the upgrade. Fixed. [1]: https://www.postgresql.org/message-id/TYAPR01MB5866D63A6460059DC661BF62F5F6A%40TYAPR01MB5866.jpnprd01.prod.outlook.com Best Regards, Hayato Kuroda FUJITSU LIMITED
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Amit,
Again, thank you for reviewing! New patch is available in [1].
> 2.
> + /*
> + * Store the names of output plugins as well. There is a possibility
> + * that duplicated plugins are set, but the consumer function
> + * check_loadable_libraries() will avoid checking the same library, so
> + * we do not have to consider their uniqueness here.
> + */
> + for (slotno = 0; slotno < slot_arr->nslots; slotno++)
> + {
> + os_info.libraries[totaltups].name =
> pg_strdup(slot_arr->slots[slotno].plugin);
>
> Here, we should ignore invalid slots.
"continue" was added.
> 3.
> + if (!live_check && !slot->caught_up)
> + {
> + if (script == NULL &&
> + (script = fopen_priv(output_path, "w")) == NULL)
> + pg_fatal("could not open file \"%s\": %s",
> + output_path, strerror(errno));
> +
> + fprintf(script,
> + "The slot \"%s\" has not consumed the WAL yet\n",
> + slot->slotname);
>
> Is it possible to print the LSN locations of slot and last checkpoint?
> I think that will aid in debugging the problems if any and could be
> helpful to users as well.
Based on recent discussion, I'm not sure we should output the actual LSN here.
(We do not check latect checkpoint anymore)
If you still think it should be, please tell me again.
[1]:
https://www.postgresql.org/message-id/TYAPR01MB5866D63A6460059DC661BF62F5F6A%40TYAPR01MB5866.jpnprd01.prod.outlook.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear hackers, > > So basically, while scanning from confirmed_flush we must ensure that > > we find a first record as SHUTDOWN CHECKPOINT record at the same LSN, > > and after that, we should not get any other WAL other than like you > > said shutdown checkpoint, running_xacts. That way we will ensure both > > aspect that the confirmed flush LSN is at the shutdown checkpoint and > > after that there is no real activity in the system. > > > > Right. > > > I think to me, > > this seems like the best available option so far. > > > > Yeah, let's see if someone else has a different opinion or has a better idea. Based on the recent discussion, I made a prototype which reads all WAL records and verifies their type. A new upgrade function binary_upgrade_validate_wal_record_types_after_lsn() does that. This function reads WALs from start_lsn (confirmed_flush), and returns true if they can ignore. The type of ignored records are listed in [1]. Kindly Hou found that XLOG_HEAP2_PRUNE may be generated during the pg_upgrade --check, so it was added to acceptable type. [1]: https://www.postgresql.org/message-id/tyapr01mb58660273eacefc5bf256b133f5...@tyapr01mb5866.jpnprd01.prod.outlook.com Best Regards, Hayato Kuroda FUJITSU LIMITED v37-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch Description: v37-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch v37-0002-Reads-all-WAL-records-ahead-confirmed_flush_lsn.patch Description: v37-0002-Reads-all-WAL-records-ahead-confirmed_flush_lsn.patch
RE: logical decoding and replication of sequences, take 2
On Wednesday, August 16, 2023 10:27 PM Tomas Vondra
wrote:
Hi,
>
>
> I guess we could update the origin, per attached 0004. We don't have
> timestamp to set replorigin_session_origin_timestamp, but it seems we don't
> need that.
>
> The attached patch merges the earlier improvements, except for the part that
> experimented with adding a "fake" transaction (which turned out to have a
> number of difficult issues).
I tried to test the patch and found a crash when calling
pg_logical_slot_get_changes() to consume sequence changes.
Steps:
create table t1_seq(a int);
create sequence seq1;
SELECT 'init' FROM pg_create_logical_replication_slot('test_slot',
'test_decoding', false, true);
INSERT INTO t1_seq SELECT nextval('seq1') FROM generate_series(1,100);
SELECT data FROM pg_logical_slot_get_changes('test_slot', NULL, NULL,
'include-xids', 'false', 'skip-empty-xacts', '1');
Attach the backtrace in bt.txt.
Best Regards,
Hou zj
(gdb) bt
#0 0x7fc7fc0ddaff in raise () from /lib64/libc.so.6
#1 0x7fc7fc0b0ea5 in abort () from /lib64/libc.so.6
#2 0x7fc7fc120097 in __libc_message () from /lib64/libc.so.6
#3 0x7fc7fc1274ec in malloc_printerr () from /lib64/libc.so.6
#4 0x7fc7fc12929c in _int_free () from /lib64/libc.so.6
#5 0x00ba3a9a in AllocSetDelete (context=0x234e820) at aset.c:652
#6 0x00bae6ab in MemoryContextDelete (context=0x234e820) at mcxt.c:437
#7 0x009171e2 in filter_by_origin_cb_wrapper (ctx=0x2356970,
origin_id=0)
at logical.c:1234
#8 0x0091153b in FilterByOrigin (ctx=0x2356970, origin_id=0) at
decode.c:583
#9 0x00911d5b in DecodeInsert (ctx=0x2356970, buf=0x7ffd28f1ad70) at
decode.c:907
#10 0x00911332 in heap_decode (ctx=0x2356970, buf=0x7ffd28f1ad70) at
decode.c:488
#11 0x00910983 in LogicalDecodingProcessRecord (ctx=0x2356970,
record=0x2356d08)
at decode.c:122
#12 0x009192af in pg_logical_slot_get_changes_guts (fcinfo=0x2389380,
confirm=true,
binary=false) at logicalfuncs.c:258
#13 0x009193e4 in pg_logical_slot_get_changes (fcinfo=0x2389380)
at logicalfuncs.c:325
#14 0x00743ab4 in ExecMakeTableFunctionResult (setexpr=0x2373828,
econtext=0x23736f8, argContext=0x2389280, expectedDesc=0x23341c8,
randomAccess=false)
at execSRF.c:235
#15 0x0075ed3d in FunctionNext (node=0x23734e8) at nodeFunctionscan.c:95
#16 0x00745312 in ExecScanFetch (node=0x23734e8, accessMtd=0x75ec8b
,
recheckMtd=0x75f095 ) at execScan.c:132
#17 0x007453b3 in ExecScan (node=0x23734e8, accessMtd=0x75ec8b
,
recheckMtd=0x75f095 ) at execScan.c:198
#18 0x0075f0df in ExecFunctionScan (pstate=0x23734e8) at
nodeFunctionscan.c:270
#19 0x00741324 in ExecProcNodeFirst (node=0x23734e8) at
execProcnode.c:464
#20 0x0073503c in ExecProcNode (node=0x23734e8)
at ../../../src/include/executor/executor.h:273
#21 0x00737ca7 in ExecutePlan (estate=0x23732c0, planstate=0x23734e8,
use_parallel_mode=false, operation=CMD_SELECT, sendTuples=true,
numberTuples=0,
direction=ForwardScanDirection, dest=0x2330518, execute_once=true) at
execMain.c:1670
#22 0x00735682 in standard_ExecutorRun (queryDesc=0x2365020,
direction=ForwardScanDirection, count=0, execute_once=true) at
execMain.c:365
#23 0x007354bf in ExecutorRun (queryDesc=0x2365020,
direction=ForwardScanDirection,
count=0, execute_once=true) at execMain.c:309
#24 0x009bfc7e in PortalRunSelect (portal=0x22df3c0, forward=true,
count=0,
dest=0x2330518) at pquery.c:924
#25 0x009bf93b in PortalRun (portal=0x22df3c0,
count=9223372036854775807,
isTopLevel=true, run_once=true, dest=0x2330518, altdest=0x2330518,
qc=0x7ffd28f1b550)
at pquery.c:768
#26 0x009b94c4 in exec_simple_query (
query_string=0x2264410 "SELECT data FROM
pg_logical_slot_get_changes('test_slot', NULL, NULL,\n'include-xids', 'false',
'skip-empty-xacts', '1');") at postgres.c:1274
#27 0x009bdb31 in PostgresMain (dbname=0x229d240 "postgres",
username=0x229d228 "houzj") at postgres.c:4637
#28 0x008f27ef in BackendRun (port=0x2293650) at postmaster.c:4433
#29 0x008f2185 in BackendStartup (port=0x2293650) at postmaster.c:4161
#30 0x008eebff in ServerLoop () at postmaster.c:1778
#31 0x008ee5cf in PostmasterMain (argc=3, argv=0x225d690) at
postmaster.c:1462
Re: bug fix and documentation improvement about vacuumdb
At Thu, 14 Sep 2023 07:57:57 -0700, Nathan Bossart wrote in > On Thu, Sep 14, 2023 at 02:06:51PM +0200, Daniel Gustafsson wrote: > > I can reproduce that, a single -N works but adding multiple -N's makes none > > of > > them excluded. The current coding does this: > > > > if (objfilter & OBJFILTER_SCHEMA_EXCLUDE) > > appendPQExpBufferStr(_query, "OPERATOR(pg_catalog.!=) "); > > > > If the join is instead made to exclude the oids in listed_objects with a > > left > > join and a clause on object_oid being null I can make the current query work > > without adding a second clause. I don't have strong feelings wrt if we > > should > > add a NOT IN () or fix this JOIN, but we shouldn't have a faulty join > > together > > with the fix. With your patch the existing join is left in place, let's fix > > that. > > Yeah, I think we can fix the JOIN as you suggest. I quickly put a patch > together to demonstrate. We should probably add some tests... It seems to work fine. However, if we're aiming for consistent spacing, the "IS NULL" (two spaces in between) might be an concern. > >> Third, for the description of the -N option, I wonder if "vacuum all > >> tables except > >> in the specified schema(s)" might be clearer. The current one says nothing > >> about > >> tables not in the specified schema. > > > > Maybe, but the point of vacuumdb is to analyze a database so I'm not sure > > who > > would expect anything else than vacuuming everything but the excluded schema > > when specifying -N. What else could "vacuumdb -N foo" be interpreted to do > > that can be confusing? > > I agree with Daniel on this one. +1. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Bug fix for psql's meta-command \ev
Hi, When a table name is specified as the first argument of \ev meta-command, it reports the error message, the prompt string becomes "-#" and then the following valid query fails because the psql's query buffer contains the garbage string generated by failure of \ev. Please see the following example. =# \ev t "public.t" is not a view -# SELECT * FROM t; ERROR: syntax error at or near "public" at character 1 STATEMENT: public.t AS SELECT * FROM t; I think this is a bug in psql's \ev meta-command. Even when \ev fails, it should not leave the garbage string in psql's query buffer and the following query should be completed successfully. This problem can be resolved by resetting the query buffer on error. You can see the attached source code. After that, it will result in output like the following: =# \ev t "public.t" is not a view =# SELECT * FROM t; i --- 1 2 (2 rows) Ryoga Yoshidadiff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index bcd8eb3538..a853bce096 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -1238,6 +1238,8 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch, else status = PSQL_CMD_NEWEDIT; } + else + resetPQExpBuffer(query_buf); free(obj_desc); }
Re: Extract numeric filed in JSONB more effectively
> Is there a reason not to transform the _tz flavors of >> jsonb_path_query and jsonb_path-query_first? >> > > I misunderstood the _tz flavors return timestamp, after some deep > reading of these functions, they just work at the comparisons part. > so I will add them in the following version. > _tz favors did return timestamp.. the reason is stated in the commit messge of patch 2. try to apply jsonb extraction optimization to _tz functions. both jsonb_path_query_tz and jsonb_path_query_tz_first returns the elements which are timestamp comparable, but such elements are impossible to be cast to numeric or boolean IIUC. Just provides this commit for communication purpose only. so v14 is attached, changes include: 1. Change the typmod for internal type from 0 to -1. 2. return NULL for non-simplify expressions from the planner support function, hence shallow copy is removed as well. Things are not addressed yet: 1. the error message handling. 2. if we have chances to optimize _tz functions, I guess no. 3. function naming issue. I think I can get it modified once after all the other issues are addressed. -- Best Regards Andy Fan v14-0002-try-to-apply-jsonb-extraction-optimization-to-_t.patch Description: Binary data v14-0001-optimize-casting-jsonb-to-a-given-type.patch Description: Binary data
Re: Is it possible to change wal_level online
Hi, On September 14, 2023 6:21:59 AM PDT, Euler Taveira wrote: >On Thu, Sep 14, 2023, at 7:05 AM, Andy Fan wrote: >> Currently it is complained that wal_level changes require an instance >> restart, I'm not familiar with this stuff so far and I didn't get any good >> information from searching the email archive. So I want to gather >> some feedbacks from experts to see if it is possible and if not, why >> it would be the key blocker for this. Basically I agree that changing >> the wal_level online will be a good experience for users. >> > >This topic was already discussed. See this thread [1] that was requesting to >change the wal_level default value. There might be other threads but I didn't >try hard to find them. > > >[1] >https://www.postgresql.org/message-id/20200608213215.mgk3cctlzvfuaqm6%40alap3.anarazel.de I think it's gotten a bit easier since then, because we now have global barriers, to implement the waiting that's mentioned in the email. Possibly we should do the switch to logical dynamically, without a dedicated wal_level. Whenever a logical slot exists, automatically increase the Wal level, whenever the last slot is dropped, lower it again. Plus some waiting to ensure every backend has knows about the new value. Regards, Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Fixup the variable name to indicate the WAL record reservation status.
At Wed, 13 Sep 2023 23:48:30 -0700, Krishnakumar R wrote in > Please find a small patch to improve code readability by fixing up the > variable name to indicate the WAL record reservation status. The > insertion is done later in the code based on the reservation status. IMHO... Although "reserved" might be pertinent at the point of assignment, its applicability promptly diminishes in the subsequent uses. When the variable is first assigned, we know the record will insert some bytes and advance the LSN. In other words, the variable suggests "to be inserted", and promptly thereafter, the variable indicates that the record "has been inserted". Given this, "inserted" seems to be a better fit than "reserved". In short, I would keep the variable name as it is. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Have better wording for snapshot file reading failure
Hi, On 2023-09-14 16:29:22 +0900, Michael Paquier wrote: > On Thu, Sep 14, 2023 at 01:33:39PM +0900, Michael Paquier wrote: > > Ahem. This seems to be the only code path that tracks a failure on > > AllocateFile() where we don't show %m at all, while the error is > > misleading in basically all the cases as errno holds the extra > > information telling somebody that something's going wrong, so I don't > > quite see how it is useful to tell "invalid snapshot identifier" on > > an EACCES or even ENOENT when opening this file, with zero information > > about what's happening on top of that? Even on ENOENT, one can be > > confused with the same error message generated a few lines above: if > > AllocateFile() fails, the snapshot identifier is correctly shaped, but > > its file is missing. If ENOENT is considered a particular case with > > the old message, we'd still not know if this refers to the first > > failure or the second failure. > > I see your point after thinking about it, the new message would show > up when running a SET TRANSACTION SNAPSHOT with a value id, which is > not helpful either. Your idea of filtering out ENOENT may be the best > move to get more information on %m. Still, it looks to me that using > the same error message for both cases is incorrect. I wouldn't call it quite incorrect, but it's certainly a good idea to provide relevant details for the rare case of errors other than ENOENT. > So, how about a "could not find the requested snapshot" if the snapshot ID > is valid but its file cannot be found? I'd probably just go for something like "snapshot \"%s\" does not exist", similar to what we report for unknown tables etc. Arguably changing the errcode to ERRCODE_UNDEFINED_OBJECT would make this more precise? > This new suggestion is only for HEAD. I've reverted a0d87bc & co for > now. I think there's really no reason to backpatch this, so that makes sense to me. Greetings, Andres Freund
Re: Support prepared statement invalidation when result types change
Hi, > > This requirement was not documented anywhere and it > can thus be a surprising error to hit. But it's actually not needed for > this to be an error, as long as we send the correct RowDescription there > does not have to be a problem for clients when the result types or > column counts change. > What if a client has *cached* an old version of RowDescription and the server changed it to something new and sent resultdata with the new RowDescription. Will the client still be able to work expectly? I don't hope my concern is right since I didn't go through any of the drivers in detail, but I hope my concern is expressed correctly. -- Best Regards Andy Fan
Re: Is it possible to change wal_level online
On Thu, Sep 14, 2023 at 9:22 PM Euler Taveira wrote: > On Thu, Sep 14, 2023, at 7:05 AM, Andy Fan wrote: > > Currently it is complained that wal_level changes require an instance > restart, I'm not familiar with this stuff so far and I didn't get any good > information from searching the email archive. So I want to gather > some feedbacks from experts to see if it is possible and if not, why > it would be the key blocker for this. Basically I agree that changing > the wal_level online will be a good experience for users. > > > This topic was already discussed. See this thread [1] that was requesting > to > change the wal_level default value. There might be other threads but I > didn't > try hard to find them. > > > [1] > https://www.postgresql.org/message-id/20200608213215.mgk3cctlzvfuaqm6%40alap3.anarazel.de > Thank you Euler, this one is already the best one I ever found. -- Best Regards Andy Fan
Re: [PATCH] Add inline comments to the pg_hba_file_rules view
On Thu, Sep 14, 2023 at 01:33:04PM +0200, Jim Jones wrote: > Just to make sure I got what you have in mind: you suggest to read the > pg_hba.conf a second time via a new (generic) function like pg_read_file() > that returns line numbers and their contents (+comments), and the results of > this new function would be joined pg_hba_file_rules in SQL. Is that correct? Yes, my suggestion was to define a new set-returning function that takes in input a file path and that returns as one row one comment and its line number from the configuration file. -- Michael signature.asc Description: PGP signature
Re: Buffer ReadMe Confuse
On Fri, Sep 15, 2023 at 4:08 AM jacktby jacktby wrote: > In buffer README, I see “Pins may not be held across transaction > boundaries, however.” I think for different transactions, they can pin the > same buffer page, why not? For concurrent read transactions, they could > read the one and the same buffer page. > > You are right that different transactions can pin the same buffer, but that does not conflict with what the README says, which is talking about once the transaction is completed, all the Pins are removed. -- Best Regards Andy Fan
Re: Cygwin cleanup
On Wed, Feb 8, 2023 at 8:06 PM Thomas Munro wrote: > On Fri, Jan 13, 2023 at 5:17 PM Justin Pryzby wrote: > > My patch used fsync_fname_ext() which would cause an ERROR rather than a > > PANIC when failing to fsync the logical decoding pathname. > > FTR While analysing a lot of CI logs trying to debug something else I > came across a plain Windows/MSVC (not Cygwin) build that panicked like > this: > > https://cirrus-ci.com/task/6689224833892352 > https://api.cirrus-ci.com/v1/artifact/task/6689224833892352/testrun/build/testrun/subscription/013_partition/log/013_partition_publisher.log > https://api.cirrus-ci.com/v1/artifact/task/6689224833892352/crashlog/crashlog-postgres.exe_0af4_2023-02-05_21-53-20-018.txt Here are some more flapping CI failures due to this phenomenon (nothing to do with Cygwin, this is just regular Windows): 4509011781877760 | Windows - Server 2019, VS 2019 - Meson & ninja 4525770962370560 | Windows - Server 2019, VS 2019 - Meson & ninja 5664518341132288 | Windows - Server 2019, VS 2019 - Meson & ninja 5689846694412288 | Windows - Server 2019, VS 2019 - Meson & ninja 5853025126842368 | Windows - Server 2019, VS 2019 - Meson & ninja 6639943179567104 | Windows - Server 2019, VS 2019 - Meson & ninja 6727728217456640 | Windows - Server 2019, VS 2019 - Meson & ninja 6740158104469504 | Windows - Server 2019, VS 2019 - Meson & ninja They all say something like 'PANIC: could not open file "pg_logical/snapshots/0-1597938.snap": No such file or directory', because they all do rename(some_temporary_file, that_name), then try to re-open and sync it, but rename() on Windows fails to be atomic so a concurrent process can see an intermediate ENOENT state. I see a few 'local' workarounds we could do to fix that, but ... there seems to be a much better idea staring us in the face in the comments! I think this would be fixed as a happy by-product of this TODO in SnapBuildSerialize(): * TODO: Do the fsync() via checkpoints/restartpoints, doing it here has * some noticeable overhead since it's performed synchronously during * decoding? I have done no analysis myself of whether that is sound, but assuming it is, I think the way to achieve that is to tweak FileTag so that it can describe the file to be fsync'd, and use the sync.c machinery to fsync the file in the background. Presumably that would provide a huge speed up for logical decoding, and people would rejoice. Some other topics that came up in this thread: * Now that PostgreSQL seems to be stable enough on Cygwin to get through the basic regression tests reliably, lorikeet might as well run the full TAP test suite? * Justin complained about the weird effects of wal_sync_method, and I finally got around to showing how I think that should be untangled, in https://commitfest.postgresql.org/44/4453/
Re: subscription TAP test has unused $result
On Thu, Sep 14, 2023 at 7:10 PM Amit Kapila wrote: > > > Though it is harmless I think we can clean it up. Your patch looks good to > > me. > > > > Pushed. > Thanks! -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Add 'worker_type' to pg_stat_subscription
On Wed, Sep 13, 2023 at 09:59:04AM -0700, Nathan Bossart wrote:
> On Wed, Sep 13, 2023 at 05:06:28PM +0300, Maxim Orlov wrote:
>> So, should we mark this thread as RfC?
>
> I've done so. Barring additional feedback, I intend to commit this in the
> next few days.
I did some staging work for the patch (attached). The one code change I
made was for the new test. Instead of adding a new test, I figured we
could modify the preceding test to check for the expected worker type
instead of whether relid is NULL. ISTM this relid check is intended to
filter for the apply worker, anyway.
The only reason I didn't apply this already is because IMHO we should
adjust the worker types and the documentation for the view to be
consistent. For example, the docs say "leader apply worker" but the view
just calls them "apply" workers. The docs say "synchronization worker" but
the view calls them "table synchronization" workers. My first instinct is
to call apply workers "leader apply" workers in the view, and to call table
synchronization workers "table synchronization workers" in the docs.
Thoughts?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From a49c8d92c4ddaf99d067d03e6adabea068497e93 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 14 Sep 2023 14:31:44 -0700
Subject: [PATCH v6 1/1] Add worker type to pg_stat_subscription.
Thanks to 2a8b40e368, the logical replication worker type is easily
determined, and this information is a nice addition to the
pg_stat_subscription view. The worker type could already be
deduced via other columns such as leader_pid and relid, but that is
unnecessary complexity for users.
Bumps catversion.
Author: Peter Smith
Reviewed-by: Michael Paquier, Maxim Orlov
Discussion: https://postgr.es/m/CAHut%2BPtmbSMfErSk0S7xxVdZJ9XVE3xVLhqBTmT91kf57BeKDQ%40mail.gmail.com
---
doc/src/sgml/monitoring.sgml | 11 +++
src/backend/catalog/system_views.sql | 1 +
src/backend/replication/logical/launcher.c | 18 +-
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.dat| 6 +++---
src/test/regress/expected/rules.out| 3 ++-
src/test/subscription/t/004_sync.pl| 2 +-
7 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 4ff415d6a0..17f9323f23 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1993,6 +1993,17 @@ description | Waiting for a newly initialized WAL file to reach durable storage
+
+
+ worker_type text
+
+
+ Type of the subscription worker process. Possible types are
+ apply, parallel, and
+ table synchronization.
+
+
+
pid integer
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 77b06e2a7a..fcb14976c0 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -949,6 +949,7 @@ CREATE VIEW pg_stat_subscription AS
SELECT
su.oid AS subid,
su.subname,
+st.worker_type,
st.pid,
st.leader_pid,
st.relid,
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 7882fc91ce..501910b445 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -1278,7 +1278,7 @@ GetLeaderApplyWorkerPid(pid_t pid)
Datum
pg_stat_get_subscription(PG_FUNCTION_ARGS)
{
-#define PG_STAT_GET_SUBSCRIPTION_COLS 9
+#define PG_STAT_GET_SUBSCRIPTION_COLS 10
Oid subid = PG_ARGISNULL(0) ? InvalidOid : PG_GETARG_OID(0);
int i;
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
@@ -1339,6 +1339,22 @@ pg_stat_get_subscription(PG_FUNCTION_ARGS)
else
values[8] = TimestampTzGetDatum(worker.reply_time);
+ switch (worker.type)
+ {
+ case WORKERTYPE_APPLY:
+values[9] = CStringGetTextDatum("apply");
+break;
+ case WORKERTYPE_PARALLEL_APPLY:
+values[9] = CStringGetTextDatum("parallel apply");
+break;
+ case WORKERTYPE_TABLESYNC:
+values[9] = CStringGetTextDatum("table synchronization");
+break;
+ case WORKERTYPE_UNKNOWN:
+/* Should never happen. */
+elog(ERROR, "unknown worker type");
+ }
+
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
values, nulls);
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 4eaef54d0c..a0c1dbca95 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -57,6 +57,6 @@
*/
/* mmddN */
-#define CATALOG_VERSION_NO 202309061
+#define CATALOG_VERSION_NO 202309141
#endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9805bc6118..f0b7b9cbd8 100644
--- a/src/include/catalog/pg_proc.dat
+++
Re: [PATCH] Add CANONICAL option to xmlserialize
On Thu, Sep 14, 2023 at 11:54 PM Jim Jones wrote: > The cfbot started complaining about this patch on "macOS - Ventura - Meson" > > 'Persistent worker failed to start the task: tart isolation failed: failed to > create VM cloned from "ghcr.io/cirruslabs/macos-ventura-base:latest": tart > command returned non-zero exit code: ""' > > Is this a problem in my code or in the CI itself? There was a temporary glitch on one of the new Mac CI runner machines that caused a few tests to fail like that, but it was fixed so that should turn red again later today.
Re: psql: Add command to use extended query protocol
In one of my environments, this feature didn't work as expected. Digging into it, I found that it is incompatible with FETCH_COUNT being set. Sorry for not recognising this during the betas. Attached a simple patch with tests running the cursor declaration through PQexecParams instead of PGexec. Alternatively, we could avoid going to ExecQueryUsingCursor and force execution via ExecQueryAndProcessResults in SendQuery (around line 1134 in src/bin/psql/common.c) when \bind is used: else if (pset.fetch_count <= 0 || pset.gexec_flag || -pset.crosstab_flag || !is_select_command(query)) +pset.crosstab_flag || !is_select_command(query) || +pset.bind_flag) best regards Tobias psql-bind-fetch_count.patch Description: Binary data
Re: Detoasting optionally to make Explain-Analyze less misleading
Hi Tom, Hi Matthias,
you are right of course. I have looked at the code from printtup.c and
made a new version of the patch.
Thanks for the MemoryContextReset hint too (@Matthias)
This time is called EXPLAIN(ANALYZE,SERIALIZE) (hey, it also sounds
nicer phonetically)
If the option SERIALIZE is active, the output functions are called and
they perform the detoasting, which I have even checked.
So things are better this way, however I hardcoded the output option
"Text" (format=0). In printtup.c there is an incoming array which
applies Text (format=0) or Binary (format=1) for each column
individually. I am not sure whether this is even needed. I left in the
if-statement from printtup.c which calls the binary output method of a
given type. The result of the output is ignored and apparently free'd
because of the memory-context-reset at the end.
Please also note, that I added a call to DestReceiver's rDestroy hook,
which was missing from explain.c before altogether.
Feedback is appreciated.
/Stepan
On 12.09.23 17:26, Tom Lane wrote:
Matthias van de Meent writes:
Hmm, maybe we should measure the overhead of serializing the tuples instead.
The difference between your patch and "serializing the tuples, but not
sending them" is that serializing also does the detoasting, but also
includes any time spent in the serialization functions of the type. So
an option "SERIALIZE" which measures all the time the server spent on
the query (except the final step of sending the bytes to the client)
would likely be more useful than "just" detoasting.
+1, that was my immediate reaction to the proposal as well. Some
output functions are far from cheap. Doing only the detoast part
seems like it's still misleading.
Do we need to go as far as offering both text-output and binary-output
options?
regards, tom lane
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 8570b14f62..dc52019a3e 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -47,7 +47,6 @@ ExplainOneQuery_hook_type ExplainOneQuery_hook = NULL;
/* Hook for plugins to get control in explain_get_index_name() */
explain_get_index_name_hook_type explain_get_index_name_hook = NULL;
-
/* OR-able flags for ExplainXMLTag() */
#define X_OPENING 0
#define X_CLOSING 1
@@ -155,6 +154,8 @@ static void ExplainYAMLLineStarting(ExplainState *es);
static void escape_yaml(StringInfo buf, const char *str);
+static DestReceiver *CreateSerializeDestReceiver(void);
+
/*
* ExplainQuery -
@@ -192,6 +193,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
es->settings = defGetBoolean(opt);
else if (strcmp(opt->defname, "generic_plan") == 0)
es->generic = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "serialize") == 0)
+ es->serialize = defGetBoolean(opt);
else if (strcmp(opt->defname, "timing") == 0)
{
timing_set = true;
@@ -244,6 +247,12 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("EXPLAIN option TIMING requires ANALYZE")));
+ /* check that serialize is used with EXPLAIN ANALYZE */
+ if (es->serialize && !es->analyze)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("EXPLAIN option SERIALIZE requires ANALYZE")));
+
/* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
if (es->generic && es->analyze)
ereport(ERROR,
@@ -565,9 +574,16 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
/*
* Normally we discard the query's output, but if explaining CREATE TABLE
* AS, we'd better use the appropriate tuple receiver.
+ * Also if we choose to serialize during explain, we need to send the
+ * tuples to a receiver that performs the serializing
+ * which includes detoasting and the conversion of the attributes
+ * into the protocol receiving format. The latter can also be costly
+ * for some types
*/
if (into)
dest = CreateIntoRelDestReceiver(into);
+ else if (es->analyze && es->serialize)
+ dest = CreateSerializeDestReceiver();
else
dest = None_Receiver;
@@ -606,6 +622,9 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
/* run cleanup too */
ExecutorFinish(queryDesc);
+ /* call the dest receiver's destroy method even during explain */
+ dest->rDestroy(dest);
+
/* We can't run ExecutorEnd 'till we're done printing the stats... */
totaltime += elapsed_time();
}
@@ -5052,3 +5071,193 @@ escape_yaml(StringInfo buf, const char *str)
{
escape_json(buf, str);
}
+
+
+/* Serializing DestReceiver functions
+ *
+ * the idea is that running EXPLAIN (ANALYZE) sometimes gives results that
+ * are way unrealistic, since during explain there is no detoasting of
+ * values. That means you get no idea what your actual query time is going
+ * to be, nor what the actual overhead of detoasting is.
+ *
+ * Also the output/send functions of some
Re: SQL:2011 application time
On 9/7/23 18:24, jian he wrote:
for a range primary key, is it fine to expect it to be unique, not
null and also not overlap? (i am not sure how hard to implement it).
-
quote from 7IWD2-02-Foundation-2011-12.pdf. 4.18.3.2 Unique
constraints, page 97 of 1483.
...
-
based on the above, the unique constraint does not specify that the
column list must be range type. UNIQUE (a, c WITHOUT OVERLAPS).
Here column "a" can be a range type (that have overlap property) and
can be not.
In fact, many of your primary key, foreign key regess test using
something like '[11,11]' (which make it more easy to understand),
which in logic is a non-range usage.
So UNIQUE (a, c WITHOUT OVERLAPS), column "a" be a non-range data type
does make sense?
I'm not sure I understand this question, but here are a few things that
might help clarify things:
In SQL:2011, a temporal primary key, unique constraint, or foreign key
may have one or more "scalar" parts (just like a regular key) followed
by one "PERIOD" part, which is denoted with "WITHOUT OVERLAPS" (in
PKs/UNIQUEs) or "PERIOD" (in FKs). Except for this last key part,
everything is still compared for equality, just as in a traditional key.
But this last part is compared for overlaps. It's exactly the same as
`EXCLUDE (id WITH =, valid_at WITH &&)`. The overlap part must come last
and you can have only one (but you may have more than one scalar part if
you like).
In the patch, I have followed that pattern, except I also allow a
regular range column anywhere I allow a PERIOD. In fact PERIODs are
mostly implemented on top of range types. (Until recently PERIOD support
was in the first patch, not the last, and there was code all throughout
for handling both, e.g. within indexes, etc. But at pgcon Peter
suggested building everything on just range columns, and then having
PERIODs create an "internal" GENERATED column, and that cleaned up the
code considerably.)
One possible source of confusion is that in the tests I'm using range
columns *also* for the scalar key part. So valid_at is a tsrange, and
int is an int4range. This is not normally how you'd use the feature, but
you need the btree_gist extension to mix int & tsrange (e.g.), and
that's not available in the regress tests. We are still comparing the
int4range for regular equality and the tsrange for overlaps. If you
search this thread there was some discussion about bringing btree_gist
into core, but it sounds like it doesn't need to happen. (It might be
still desirable independently. EXCLUDE constraints are also not really
something you can use practically without it, and their tests use the
same trick of comparing ranges for plain equality.)
The piece of discussion you're replying to is about allowing *multiple*
WITHOUT OVERLAPS modifiers on a PK/UNIQUE constraint, and in any
position. I think that's a good idea, so I've started adapting the code
to support it. (In fact there is a lot of code that assumes the overlaps
key part will be in the last position, and I've never really been happy
with that, so it's an excuse to make that more robust.) Here I'm saying
(1) you will still need at least one scalar key part, (2) if there are
no WITHOUT OVERLAPS parts then you just have a regular key, not a
temporal one, (3) changing this obliges us to do the same for foreign
keys and FOR PORTION OF.
I hope that helps! I apologize if I've completely missed the point. If
so please try again. :-)
Yours,
--
Paul ~{:-)
p...@illuminatedcomputing.com
Buffer ReadMe Confuse
In buffer README, I see “Pins may not be held across transaction boundaries, however.” I think for different transactions, they can pin the same buffer page, why not? For concurrent read transactions, they could read the one and the same buffer page.
Re: bug fix and documentation improvement about vacuumdb
On Thu, Sep 14, 2023 at 02:06:51PM +0200, Daniel Gustafsson wrote:
>> On 14 Sep 2023, at 13:21, Kuwamura Masaki
>> wrote:
>
>> PATTERN should be changed to SCHEMA because -n and -N options don't support
>> pattern matching for schema names. The attached patch 0001 fixes this.
>
> True, there is no pattern matching performed. I wonder if it's worth lifting
> the pattern matching from pg_dump into common code such that tools like this
> can use it?
I agree that this should be changed to SCHEMA. It might be tough to add
pattern matching with the current catalog query, and I don't know whether
there is demand for such a feature, but I wouldn't discourage someone from
trying.
>> Second, when we use multiple -N options, vacuumdb runs incorrectly as shown
>> below.
>> ...
>
>> Even specified by -N, s1.t and s2.t are vacuumed, and also the others are
>> vacuumed
>> twice. The attached patch 0002 fixes this.
>
> I can reproduce that, a single -N works but adding multiple -N's makes none of
> them excluded. The current coding does this:
>
> if (objfilter & OBJFILTER_SCHEMA_EXCLUDE)
> appendPQExpBufferStr(_query, "OPERATOR(pg_catalog.!=) ");
>
> If the join is instead made to exclude the oids in listed_objects with a left
> join and a clause on object_oid being null I can make the current query work
> without adding a second clause. I don't have strong feelings wrt if we should
> add a NOT IN () or fix this JOIN, but we shouldn't have a faulty join together
> with the fix. With your patch the existing join is left in place, let's fix
> that.
Yeah, I think we can fix the JOIN as you suggest. I quickly put a patch
together to demonstrate. We should probably add some tests...
>> Third, for the description of the -N option, I wonder if "vacuum all tables
>> except
>> in the specified schema(s)" might be clearer. The current one says nothing
>> about
>> tables not in the specified schema.
>
> Maybe, but the point of vacuumdb is to analyze a database so I'm not sure who
> would expect anything else than vacuuming everything but the excluded schema
> when specifying -N. What else could "vacuumdb -N foo" be interpreted to do
> that can be confusing?
I agree with Daniel on this one.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index f03d5b1c6c..5e05f27462 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -662,18 +662,19 @@ vacuum_one_database(ConnParams *cparams,
/* Used to match the tables or schemas listed by the user */
if (objects_listed)
{
- appendPQExpBufferStr(_query, " JOIN listed_objects"
- " ON listed_objects.object_oid ");
-
- if (objfilter & OBJFILTER_SCHEMA_EXCLUDE)
- appendPQExpBufferStr(_query, "OPERATOR(pg_catalog.!=) ");
- else
- appendPQExpBufferStr(_query, "OPERATOR(pg_catalog.=) ");
+ appendPQExpBufferStr(_query, " LEFT JOIN listed_objects"
+ " ON listed_objects.object_oid"
+ " OPERATOR(pg_catalog.=) ");
if (objfilter & OBJFILTER_TABLE)
appendPQExpBufferStr(_query, "c.oid\n");
else
appendPQExpBufferStr(_query, "ns.oid\n");
+
+ appendPQExpBuffer(_query,
+ " WHERE listed_objects.object_oid IS %s NULL\n",
+ (objfilter & OBJFILTER_SCHEMA_EXCLUDE) ? "" : "NOT");
+ has_where = true;
}
/*
@@ -684,9 +685,11 @@ vacuum_one_database(ConnParams *cparams,
*/
if ((objfilter & OBJFILTER_TABLE) == 0)
{
- appendPQExpBufferStr(_query, " WHERE c.relkind OPERATOR(pg_catalog.=) ANY (array["
- CppAsString2(RELKIND_RELATION) ", "
- CppAsString2(RELKIND_MATVIEW) "])\n");
+ appendPQExpBuffer(_query,
+ " %s c.relkind OPERATOR(pg_catalog.=) ANY (array["
+ CppAsString2(RELKIND_RELATION) ", "
+ CppAsString2(RELKIND_MATVIEW) "])\n",
+ has_where ? "AND" : "WHERE");
has_where = true;
}
Re: ALTER COLUMN ... SET EXPRESSION to alter stored generated column's expression
Hi Amul, I share others opinion that this feature is useful. >> On Fri, 25 Aug 2023 at 03:06, Vik Fearing wrote: >>> >>> >>> I don't like this part of the patch at all. Not only is the >>> documentation only half baked, but the entire concept of the two >>> commands is different. Especially since I believe the command should >>> also create a generated column from a non-generated one. >> >> >> But I have to agree with Vik Fearing, we can make this patch better, should >> we? >> I totally understand your intentions to keep the code flow simple and reuse >> existing code as much >> as possible. But in terms of semantics of these commands, they are quite >> different from each other. >> And in terms of reading of the code, this makes it even harder to understand >> what is going on here. >> So, in my view, consider split these commands. > > > Ok, probably, I would work in that direction. I did the same thing that > SET/DROP DEFAULT does, despite semantic differences, and also, if I am not > missing anything, the code complexity should be the same as that. If we allow SET EXPRESSION to convert a non-generated column to a generated one, the current way of handling ONLY would yield mismatch between parent and child. That's not allowed as per the documentation [1]. In that sense not allowing SET to change the GENERATED status is better. I think that can be added as a V2 feature, if it overly complicates the patch Or at least till a point that becomes part of SQL standard. I think V1 patch can focus on changing the expression of a column which is already a generated column. Regarding code, I think we should place it where it's reasonable - following precedence is usually good. But I haven't reviewed the code to comment on it. [1] https://www.postgresql.org/docs/16/ddl-generated-columns.html -- Best Wishes, Ashutosh Bapat
Re: Is it possible to change wal_level online
On Thu, Sep 14, 2023, at 7:05 AM, Andy Fan wrote: > Currently it is complained that wal_level changes require an instance > restart, I'm not familiar with this stuff so far and I didn't get any good > information from searching the email archive. So I want to gather > some feedbacks from experts to see if it is possible and if not, why > it would be the key blocker for this. Basically I agree that changing > the wal_level online will be a good experience for users. > This topic was already discussed. See this thread [1] that was requesting to change the wal_level default value. There might be other threads but I didn't try hard to find them. [1] https://www.postgresql.org/message-id/20200608213215.mgk3cctlzvfuaqm6%40alap3.anarazel.de -- Euler Taveira EDB https://www.enterprisedb.com/
Re: What's the eviction algorithm of newest pg version?
On Fri, 15 Sept 2023 at 00:53, jacktby jacktby wrote: > A normal LRU cache or implement it reference to a research paper? src/backend/storage/buffer/README David
Re: bug fix and documentation improvement about vacuumdb
> On 14 Sep 2023, at 13:21, Kuwamura Masaki > wrote: > PATTERN should be changed to SCHEMA because -n and -N options don't support > pattern matching for schema names. The attached patch 0001 fixes this. True, there is no pattern matching performed. I wonder if it's worth lifting the pattern matching from pg_dump into common code such that tools like this can use it? > Second, when we use multiple -N options, vacuumdb runs incorrectly as shown > below. > ... > Even specified by -N, s1.t and s2.t are vacuumed, and also the others are > vacuumed > twice. The attached patch 0002 fixes this. I can reproduce that, a single -N works but adding multiple -N's makes none of them excluded. The current coding does this: if (objfilter & OBJFILTER_SCHEMA_EXCLUDE) appendPQExpBufferStr(_query, "OPERATOR(pg_catalog.!=) "); If the join is instead made to exclude the oids in listed_objects with a left join and a clause on object_oid being null I can make the current query work without adding a second clause. I don't have strong feelings wrt if we should add a NOT IN () or fix this JOIN, but we shouldn't have a faulty join together with the fix. With your patch the existing join is left in place, let's fix that. > Third, for the description of the -N option, I wonder if "vacuum all tables > except > in the specified schema(s)" might be clearer. The current one says nothing > about > tables not in the specified schema. Maybe, but the point of vacuumdb is to analyze a database so I'm not sure who would expect anything else than vacuuming everything but the excluded schema when specifying -N. What else could "vacuumdb -N foo" be interpreted to do that can be confusing? -- Daniel Gustafsson
Re: [PATCH] Add CANONICAL option to xmlserialize
The cfbot started complaining about this patch on "macOS - Ventura - Meson" 'Persistent worker failed to start the task: tart isolation failed: failed to create VM cloned from "ghcr.io/cirruslabs/macos-ventura-base:latest": tart command returned non-zero exit code: ""' Is this a problem in my code or in the CI itself? Thanks! Jim
What's the eviction algorithm of newest pg version?
A normal LRU cache or implement it reference to a research paper?
Re: [PATCH] Add inline comments to the pg_hba_file_rules view
Hi On 11.09.23 00:33, Michael Paquier wrote: Well, it looks like what I wrote a couple of days ago was perhaps confusing: https://www.postgresql.org/message-id/ZPHAiNp%2ByKMsa/vc%40paquier.xyz https://www.postgresql.org/message-id/zpe8a7enuh+ax...@paquier.xyz This patch touches hbafuncs.c and the system view pg_hba_file_rules, but I don't think this stuff should touch any of these code paths. That's what I meant in my second message: the SQL portion should be usable for all types of configuration files, even pg_ident.conf and postgresql.conf, and not only pg_hba.conf. A new SQL function returning a SRF made of the comments extracted and the line numbers can be joined with all the system views of the configuration files, like sourcefile and sourceline in pg_settings, etc. -- Michael Thanks for the feedback. I indeed misunderstood what you meant in the other thread, as you explicitly only mentioned hba.c. The change to hbafunc.c was mostly a function call and a new column to the view: comment = GetInlineComment(hba->rawline); if(comment) values[index++] = CStringGetTextDatum(comment); else nulls[index++] = true; Just to make sure I got what you have in mind: you suggest to read the pg_hba.conf a second time via a new (generic) function like pg_read_file() that returns line numbers and their contents (+comments), and the results of this new function would be joined pg_hba_file_rules in SQL. Is that correct? Thanks
bug fix and documentation improvement about vacuumdb
Hi there, I have 1 trivial fix, 1 bug fix, and 1 suggestion about vacuumdb. First, I noticed that the help message of `vacuumdb` is a bit incorrect. `vacuumdb -?` displays the following message ``` ... -n, --schema=PATTERNvacuum tables in the specified schema(s) only -N, --exclude-schema=PATTERNdo not vacuum tables in the specified schema(s) ... ``` PATTERN should be changed to SCHEMA because -n and -N options don't support pattern matching for schema names. The attached patch 0001 fixes this. Second, when we use multiple -N options, vacuumdb runs incorrectly as shown below. ``` $ psql =# CREATE SCHEMA s1; =# CREATE SCHEMA s2; =# CREATE SCHEMA s3; =# CREATE TABLE s1.t(i int); =# CREATE TABLE s2.t(i int); =# CREATE TABLE s3.t(i int); =# ALTER SYSTEM SET log_statement TO 'all'; =# SELECT pg_reload_conf(); =# \q $ vacuumdb -N s1 -N s2 ``` We expect that tables in schemas s1 and s2 should not be vacuumed, while the others should be. However, logfile says like this. ``` LOG: statement: VACUUM (SKIP_DATABASE_STATS) pg_catalog.pg_proc; LOG: statement: VACUUM (SKIP_DATABASE_STATS) pg_catalog.pg_proc; ... LOG: statement: VACUUM (SKIP_DATABASE_STATS) s2.t; LOG: statement: VACUUM (SKIP_DATABASE_STATS) s1.t; LOG: statement: VACUUM (ONLY_DATABASE_STATS); ``` Even specified by -N, s1.t and s2.t are vacuumed, and also the others are vacuumed twice. The attached patch 0002 fixes this. Third, for the description of the -N option, I wonder if "vacuum all tables except in the specified schema(s)" might be clearer. The current one says nothing about tables not in the specified schema. Thoughts? Masaki Kuwamura v1-0001-vacuumdb-Fix-help-message.patch Description: Binary data v1-0002-vacuumdb-Fix-bug-multiple-N-switches.patch Description: Binary data
Re: Reducing memory consumed by RestrictInfo list translations in partitionwise join planning
Hi All,
On Fri, Aug 11, 2023 at 6:24 PM Ashutosh Bapat
wrote:
>
> Obtaining child clauses from parent clauses by translation and
> tracking the translations is less complex and may be more efficient
> too. I will post a patch on those lines soon.
>
PFA patch set to add infrastructure to track RestrictInfo translations
and reuse them.
PlannerInfo gets a new member "struct HTAB *child_rinfo_hash" which is
a hash table of hash tables keyed by RestrictInfo::rinfo_serial. Each
element in the array is a hash table of RestrictInfos keyed by
RestrictInfo::required_relids as explained in my previous email. When
building child clauses when a. building child join rels or b. when
reparameterizing paths, we first access the first level hash table
using RestrictInfo::rinfo_serial of the parent and search the required
translation by computing the child RestrictInfo::required_relids
obtained by translating RestrictInfo::required_relids of the parent
RestrictInfo. If the translation doesn't exist, we create one and add
to the hash table.
RestrictInfo::required_relids is same for a RestrictInfo and its
commuted RestrictInfo. The order of operands is important for
IndexClauses. Hence we track the commuted RestrictInfo in a new field
RestrictInfo::comm_rinfo. RestrictInfo::is_commuted differentiates
between a RestrictInfo and its commuted version. This is explained as
a comment in the patch. This scheme has a minor benefit of saving
memory when the same RestrictInfo is commuted multiple times.
Hash table of hash table is used instead of an array of hash tables
since a. not every rinfo_serial has a RestrictInfo associated with it
b. not every RestrictInfo has translations, c. I don't think the exact
size of this array is not known till the planning ends since we
continue to create new clauses as we create new RelOptInfos. Of
course, an array can be repalloc'ed and unused slots in the array may
not waste a lot of memory. I am open to change hash table to an array
which may be more efficient.
With these set of patches, the memory consumption stands as below
Number of tables | without patch | with patch | % reduction |
being joined ||| |
--
2 | 40.8 MiB | 37.4 MiB | 8.46% |
3 | 151.6 MiB | 135.0 MiB | 10.96% |
4 | 464.0 MiB | 403.6 MiB | 12.00% |
5 |1663.9 MiB | 1329.1 MiB | 20.12% |
The patch set is thus
0001 - patch used to measure memory used during planning
0002 - Patch to free intermediate Relids computed by
adjust_child_relids_multilevel(). I didn't test memory consumption for
multi-level partitioning. But this is clear improvement. In that
function we free the AppendInfos array which as many pointers long as
the number of relids. So it doesn't make sense not to free the Relids
which can be {largest relid}/8 bytes long at least.
0003 - patch to save and reuse commuted RestrictInfo. This patch by
itself shows a small memory saving (3%) in the query below where the
same clause is commuted twice. The query does not contain any
partitioned tables.
create table t2 (a int primary key, b int, c int);
create index t2_a_b on t2(a, b);
select * from t2 where 10 = a
Memory used without patch: 13696 bytes
Memory used with patch: 13264 bytes
0004 - Patch which implements the hash table of hash table described
above and also code to avoid repeated RestrictInfo list translations.
I will add this patchset to next commitfest.
--
Best Wishes,
Ashutosh Bapat
From 3b9e8039c15c87f6dc83369419c85c4a0a3b5688 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat
Date: Thu, 31 Aug 2023 19:02:23 +0530
Subject: [PATCH 2/4] Free intermediate Relids created in
adjust_child_relids_multilevel()
adjust_child_relids_multilevel() creates Relids for all the intermediate stages
in a partition hierarchy. These relid sets are not required finally. Relids or
Bitmapset take reasonably large space when thousands of partitions are
invovled. Hence free these intermediate relid sets.
Ashutosh Bapat
---
src/backend/optimizer/util/appendinfo.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/backend/optimizer/util/appendinfo.c b/src/backend/optimizer/util/appendinfo.c
index f456b3b0a4..4008e52de2 100644
--- a/src/backend/optimizer/util/appendinfo.c
+++ b/src/backend/optimizer/util/appendinfo.c
@@ -591,6 +591,8 @@ adjust_child_relids_multilevel(PlannerInfo *root, Relids relids,
{
AppendRelInfo **appinfos;
int nappinfos;
+ Relids tmp_relids = NULL;
+ Relids child_relids;
/*
* If the given relids set doesn't contain any of the parent relids, it
@@ -599,13 +601,14 @@ adjust_child_relids_multilevel(PlannerInfo *root, Relids relids,
if (!bms_overlap(relids, parentrel->relids))
return relids;
+ tmp_relids = relids;
/* Recurse if immediate parent is not the top parent. */
if (childrel->parent
Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)
Em qua., 13 de set. de 2023 às 22:32, Michael Paquier escreveu: > On Wed, Sep 13, 2023 at 08:14:11AM -0700, Jeff Davis wrote: > > Looks good to me, thank you. > > Applied, then. Thanks. > Thank you Michael, for the commit. best regards, Ranier Vilela
Re: Index range search optimization
Hi!
On Fri, Jun 23, 2023 at 10:36 AM Konstantin Knizhnik
wrote:
> _bt_readpage performs key check for each item on the page trying to locate
> upper boundary.
> While comparison of simple integer keys are very fast, comparison of long
> strings can be quite expensive.
> We can first make check for the largest key on the page and if it is not
> larger than upper boundary, then skip checks for all elements.
>
> At this quite artificial example such optimization gives 3x time speed-up:
>
> create table t(t text primary key);
> insert into t values ('primary key-'||generate_series(1,1000)::text);
> select count(*) from t where t between 'primary key-100' and 'primary
> key-200';
>
> At my notebook with large enough shared buffers and disabled concurrency
> the difference is 83 vs. 247 msec
> For integer keys the difference is much smaller: 69 vs. 82 msec
>
> Certainly I realized that this example is quite exotic: most of DBAs
> prefer integer keys and such large ranges are quite rare.
> But still such large range queries are used.
> And I have checked that the proposed patch doesn't cause slowdown of exact
> search.
>
Neat optimization! But I wonder if we could do even better. The attached
patch allows Postgres to skip scan keys required for directional scans
(even when other keys are present in the scan). I'll soon post the testing
results and a more polished version of this patch.
--
Regards,
Alexander Korotkov
0001-Skip-checking-of-scan-keys-required-for-direction-v1.patch
Description: Binary data
Is it possible to change wal_level online
Hi, Currently it is complained that wal_level changes require an instance restart, I'm not familiar with this stuff so far and I didn't get any good information from searching the email archive. So I want to gather some feedbacks from experts to see if it is possible and if not, why it would be the key blocker for this. Basically I agree that changing the wal_level online will be a good experience for users. -- Best Regards Andy Fan
Re: CHECK Constraint Deferrable
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: [PATCH] Add native windows on arm64 support
> On 13 Sep 2023, at 21:12, Peter Eisentraut wrote: > > On 31.08.23 06:44, Tom Lane wrote: >> I agree. I'm really uncomfortable with claiming support for >> Windows-on-ARM if we don't have a buildfarm member testing it. >> For other platforms that have a track record of multiple >> hardware support, it might not be a stretch ... but Windows was >> so resolutely Intel-only for so long that "it works on ARM" is >> a proposition that I won't trust without hard evidence. There >> are too many bits of that system that might not have gotten the >> word yet, or at least not gotten sufficient testing. >> My vote for this is we don't commit without a buildfarm member. > > I think we can have a multi-tiered approach, where we can commit support but > consider it experimental until we have buildfarm coverage. If it's experimental it should probably be behind an opt-in flag in autoconf/meson, or be reverted by the time REL_17_STABLE branches unless coverage has materialized by then. -- Daniel Gustafsson
Re: Small patch modifying variable name to reflect the logic involved
> On 14 Sep 2023, at 08:28, Krishnakumar R wrote: > Please find a small patch to improve code readability by modifying > variable name to reflect the logic involved - finding diff between end > and start time of WAL sync. - INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_sync_time, duration, start); + INSTR_TIME_SET_CURRENT(end); + INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_sync_time, end, start); Agreed, the duration is the result of the INSTR_TIME_ACCUM_DIFF calculation, not what's stored in the instr_time variable. -- Daniel Gustafsson
Re: Quoting filename in using facing log messages
> On 14 Sep 2023, at 09:56, Michael Paquier wrote: > (I'm OK with your patch as well, FWIW.) Thanks for looking, pushed. -- Daniel Gustafsson
Re: persist logical slots to disk during shutdown checkpoint
On Thu, Sep 14, 2023 at 7:20 AM Michael Paquier wrote: > > On Wed, Sep 13, 2023 at 04:20:37PM +0530, Amit Kapila wrote: > > The patch is updated as per recent discussion. > > WFM. Thanks for the updated version. > Pushed. -- With Regards, Amit Kapila.
Re: subscription TAP test has unused $result
On Wed, Sep 13, 2023 at 10:14 AM Amit Kapila wrote: > > On Wed, Sep 13, 2023 at 8:43 AM Peter Smith wrote: > > > > Yesterday noticed a TAP test assignment to an unused $result. > > > > PSA patch to remove that. > > > > Though it is harmless I think we can clean it up. Your patch looks good to me. > Pushed. -- With Regards, Amit Kapila.
Re: ALTER COLUMN ... SET EXPRESSION to alter stored generated column's expression
On Wed, Sep 13, 2023 at 2:28 PM Maxim Orlov wrote: > Hi! > > I'm pretty much like the idea of the patch. Looks like an overlook in SQL > standard for me. > Anyway, patch apply with no conflicts and implements described > functionality. > > Thank you for looking at this. > On Fri, 25 Aug 2023 at 03:06, Vik Fearing wrote: > >> >> I don't like this part of the patch at all. Not only is the >> documentation only half baked, but the entire concept of the two >> commands is different. Especially since I believe the command should >> also create a generated column from a non-generated one. > > > But I have to agree with Vik Fearing, we can make this patch better, > should we? > I totally understand your intentions to keep the code flow simple and reuse > existing code as much > as possible. But in terms of semantics of these commands, they are quite > different from each other. > And in terms of reading of the code, this makes it even harder to > understand what is going on here. > So, in my view, consider split these commands. > Ok, probably, I would work in that direction. I did the same thing that SET/DROP DEFAULT does, despite semantic differences, and also, if I am not missing anything, the code complexity should be the same as that. Regards, Amul
Re: Reducing connection overhead in pg_upgrade compat check phase
> On 13 Sep 2023, at 16:12, Peter Eisentraut wrote: > The alignment of this output looks a bit funny: > > ... > Checking for prepared transactionsok > Checking for contrib/isn with bigint-passing mismatch ok > Checking for data type usage checking all > databases > ok > Checking for presence of required libraries ok > Checking database user is the install userok > ... I was using the progress reporting to indicate that it hadn't stalled for slow systems, but it's not probably not all that important really. Removed such that "ok" aligns. > Also, you should put gettext_noop() calls into the .status = "Checking ..." > assignments and arrange to call gettext() where they are used, to maintain > the translatability. Ah, yes of course. Fixed. -- Daniel Gustafsson v9-0001-pg_upgrade-run-all-data-type-checks-per-connectio.patch Description: Binary data
Re: Redundant Unique plan node for table with a unique index
Thank you for feedback and thread [1]. Regards, Damir Belyalov Postgres Professional
Re: [dynahash] do not refill the hashkey after hash_search
On Wed, Sep 13, 2023 at 5:28 PM John Naylor
wrote:
>
>
> On Wed, Sep 13, 2023 at 3:46 PM Junwang Zhao wrote:
> >
> > On Wed, Sep 13, 2023 at 4:22 PM John Naylor
> > wrote:
>
> > > - memset(part_entry, 0, sizeof(LogicalRepPartMapEntry));
> > > - part_entry->partoid = partOid;
> > > + Assert(part_entry->partoid == partOid);
> > > + memset(entry, 0, sizeof(LogicalRepRelMapEntry));
> > >
> > > This is making an assumption that the non-key part of
> > > LogicalRepPartMapEntry will never get new members. Without knowing much
> > > about this code, it seems like a risk in the abstract.
> >
> > What do you mean by 'the non-key part of LogicalRepPartMapEntry will
> > never get new members'?
>
> I mean, if this struct:
>
> > typedef struct LogicalRepPartMapEntry
> > {
> > Oid partoid; /* LogicalRepPartMap's key */
> > LogicalRepRelMapEntry relmapentry;
> > } LogicalRepPartMapEntry;
>
> ...gets a new member, it will not get memset when memsetting "relmapentry".
ok, I see. I will leave this case as it was.
>
> > > Taking a quick look, I didn't happen to see any existing asserts of this
> > > sort, so the patch doesn't seem to be making things more "normal". I did
> > > see a few instances of /* hash_search already filled in the key */, so if
> > > we do anything at all here, we might prefer that.
> >
> > There are some code using assert for this sort, for example in
> > *ReorderBufferToastAppendChunk*:
>
> > and in *rebuild_database_list*, tom commented that the key has already
> > been filled, which I think
> > he was trying to tell people no need to assign the key again.
>
> Okay, we have examples of each.
>
> --
> John Naylor
> EDB: http://www.enterprisedb.com
Add a v2 with some change to fix warnings about unused-parameter.
I will add this to Commit Fest.
--
Regards
Junwang Zhao
v2-0001-do-not-refill-the-hashkey-after-hash_search.patch
Description: Binary data
Re: information_schema and not-null constraints
On 06.09.23 19:52, Alvaro Herrera wrote:
+SELECT current_database()::information_schema.sql_identifier AS
constraint_catalog,
+ rs.nspname::information_schema.sql_identifier AS constraint_schema,
+ con.conname::information_schema.sql_identifier AS constraint_name,
+ format('CHECK (%s IS NOT NULL)',
at.attname)::information_schema.character_data AS check_clause
Small correction here: This should be
pg_catalog.format('%s IS NOT NULL',
at.attname)::information_schema.character_data AS check_clause
That is, the word "CHECK" and the parentheses should not be part of the
produced value.
Re: Cleaning up array_in()
On Thu, Sep 14, 2023 at 2:00 PM Alexander Lakhin wrote:
>
>
> I didn't mean to remove the prefix "array_in-", but in fact I was confused
> by the "{function_name}-" syntax, and now when I've looked at it closely, I
> see that that syntax was quite popular ("date_in- ", "single_decode- ", ...)
> back in 1997 (see 9d8ae7977). But nowadays it is out of fashion, with most
> of such debugging prints were gone with 7a877dfd2 and the next-to-last one
> with 50861cd68. Moreover, as the latter commit shows, such debugging output
> can be eliminated completely without remorse. (And I couldn't find mentions
> of ARRAYDEBUG in pgsql-bugs, pgsql-hackers archives, so probably no one used
> that debugging facility since it's introduction.)
> As of now, the output still weird (I mean the excessive right parenthesis):
> DEBUG: ndim 1 ( 2 -1 -1 -1 -1 -1); lBound info: 1 1 1 1 1 1) for {0,0}
>
hi.
similar to NUMERIC_DEBUG. I made the following adjustments.
if unnecessary, removing this part seems also fine, in GDB, you can
print it out directly.
/* --
* Uncomment the following to get a dump of a array's ndim, dim, lBound.
* --
#define ARRAYDEBUG
*/
#ifdef ARRAYDEBUG
{
StringInfoData buf;
initStringInfo();
appendStringInfo(, "array_in- ndim %d, dim info(", ndim);
for (int i = 0; i < MAXDIM; i++)
appendStringInfo(, " %d", dim[i]);
appendStringInfo(, "); lBound info(");
for (int i = 0; i < MAXDIM; i++)
appendStringInfo(, " %d", lBound[i]);
appendStringInfo(, ") for %s", string);
elog(DEBUG1, "%s", buf.data);
pfree(buf.data);
}
#endif
other than this, no other changes.
From 84b285b3ecc5461cc56dec455e3b3bb77d4104ad Mon Sep 17 00:00:00 2001
From: Tom Lane
Date: Tue, 4 Jul 2023 12:39:41 -0400
Subject: [PATCH v9 1/7] Simplify and speed up ReadArrayStr().
ReadArrayStr() seems to have been written on the assumption that
non-rectangular input is fine and it should pad with NULLs anywhere
that elements are missing. We disallowed non-rectangular input
ages ago (commit 0e13d627b), but never simplified this function
as a follow-up. In particular, the existing code recomputes each
element's linear location from scratch, which is quite unnecessary
for rectangular input: we can just assign the elements sequentially,
saving lots of arithmetic. Add some more commentary while at it.
ArrayGetOffset0() is no longer used anywhere, so remove it.
---
src/backend/utils/adt/arrayfuncs.c | 69 ++
src/backend/utils/adt/arrayutils.c | 15 ---
src/include/utils/array.h | 1 -
3 files changed, 33 insertions(+), 52 deletions(-)
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 7828a626..df1ebb47 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -93,7 +93,7 @@ typedef struct ArrayIteratorData
static int ArrayCount(const char *str, int *dim, char typdelim,
Node *escontext);
static bool ReadArrayStr(char *arrayStr, const char *origStr,
- int nitems, int ndim, int *dim,
+ int nitems,
FmgrInfo *inputproc, Oid typioparam, int32 typmod,
char typdelim,
int typlen, bool typbyval, char typalign,
@@ -391,7 +391,7 @@ array_in(PG_FUNCTION_ARGS)
dataPtr = (Datum *) palloc(nitems * sizeof(Datum));
nullsPtr = (bool *) palloc(nitems * sizeof(bool));
if (!ReadArrayStr(p, string,
- nitems, ndim, dim,
+ nitems,
_extra->proc, typioparam, typmod,
typdelim,
typlen, typbyval, typalign,
@@ -436,7 +436,8 @@ array_in(PG_FUNCTION_ARGS)
/*
* ArrayCount
- * Determines the dimensions for an array string.
+ * Determines the dimensions for an array string. This includes
+ * syntax-checking the array structure decoration (braces and delimiters).
*
* Returns number of dimensions as function result. The axis lengths are
* returned in dim[], which must be of size MAXDIM.
@@ -683,16 +684,14 @@ ArrayCount(const char *str, int *dim, char typdelim, Node *escontext)
/*
* ReadArrayStr :
* parses the array string pointed to by "arrayStr" and converts the values
- * to internal format. Unspecified elements are initialized to nulls.
- * The array dimensions must already have been determined.
+ * to internal format. The array dimensions must have been determined,
+ * and the case of an empty array must have been handled earlier.
*
* Inputs:
* arrayStr: the string to parse.
* CAUTION: the contents of "arrayStr" will be modified!
* origStr: the unmodified input string, used only in error messages.
* nitems: total number of array elements, as already determined.
- * ndim: number of array dimensions
- * dim[]: array axis lengths
* inputproc: type-specific input procedure for element datatype.
* typioparam, typmod: auxiliary values to pass to inputproc.
* typdelim: the value delimiter (type-specific).
@@ -717,8 +716,6 @@ static bool
ReadArrayStr(char *arrayStr,
const char *origStr,
int nitems,
- int
Re: Quoting filename in using facing log messages
On Wed, Sep 13, 2023 at 02:02:47PM +0200, Daniel Gustafsson wrote:
> It might be worth concatenating the errmsg() while there since we typically
> don't linebreak errmsg strings anymore for greppability:
>
> - errmsg("could not write to log file %s "
> - "at offset %u, length %zu: %m",
> + errmsg("could not write to log file \"%s\" at offset %u, length %zu:
> %m",
>
> I don't have strong feelings wrt that, just have a vague memory of
> "concatenate
> when touching" as an informal guideline.
Because these are slightly easier to grep when looking for a given
pattern in the tree.
(I'm OK with your patch as well, FWIW.)
--
Michael
signature.asc
Description: PGP signature
Re: Have better wording for snapshot file reading failure
On Thu, Sep 14, 2023 at 01:33:39PM +0900, Michael Paquier wrote:
> Ahem. This seems to be the only code path that tracks a failure on
> AllocateFile() where we don't show %m at all, while the error is
> misleading in basically all the cases as errno holds the extra
> information telling somebody that something's going wrong, so I don't
> quite see how it is useful to tell "invalid snapshot identifier" on
> an EACCES or even ENOENT when opening this file, with zero information
> about what's happening on top of that? Even on ENOENT, one can be
> confused with the same error message generated a few lines above: if
> AllocateFile() fails, the snapshot identifier is correctly shaped, but
> its file is missing. If ENOENT is considered a particular case with
> the old message, we'd still not know if this refers to the first
> failure or the second failure.
I see your point after thinking about it, the new message would show
up when running a SET TRANSACTION SNAPSHOT with a value id, which is
not helpful either. Your idea of filtering out ENOENT may be the best
move to get more information on %m. Still, it looks to me that using
the same error message for both cases is incorrect. So, how about a
"could not find the requested snapshot" if the snapshot ID is valid
but its file cannot be found? We don't have any tests for the failure
paths, either, so I've added some.
This new suggestion is only for HEAD. I've reverted a0d87bc & co for
now.
--
Michael
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index b20440ee21..3040873672 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1390,9 +1390,21 @@ ImportSnapshot(const char *idstr)
f = AllocateFile(path, PG_BINARY_R);
if (!f)
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("invalid snapshot identifier: \"%s\"", idstr)));
+ {
+ /*
+ * If file is missing while identifier has a correct format, avoid
+ * system errors.
+ */
+ if (errno == ENOENT)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not find the requested snapshot: \"%s\"", idstr)));
+ else
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not open file \"%s\" for reading: %m",
+ path)));
+ }
/* get the size of the file so that we know how much memory we need */
if (fstat(fileno(f), _buf))
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 428c9edcc6..7a674b2156 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -1148,6 +1148,17 @@ SELECT * FROM trans_abc ORDER BY 1;
(3 rows)
DROP TABLE trans_abc;
+-- TRANSACTION SNAPSHOT
+-- Incorrect identifier.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'Incorrect Identifier';
+ERROR: invalid snapshot identifier: "Incorrect Identifier"
+ROLLBACK;
+-- Correct identifier, missing file.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'FFF-FFF-F';
+ERROR: could not find the requested snapshot: "FFF-FFF-F"
+ROLLBACK;
-- Test for successful cleanup of an aborted transaction at session exit.
-- THIS MUST BE THE LAST TEST IN THIS FILE.
begin;
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index 75ffe929d4..db2535c787 100644
--- a/src/test/regress/sql/transactions.sql
+++ b/src/test/regress/sql/transactions.sql
@@ -613,6 +613,15 @@ SELECT * FROM trans_abc ORDER BY 1;
DROP TABLE trans_abc;
+-- TRANSACTION SNAPSHOT
+-- Incorrect identifier.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'Incorrect Identifier';
+ROLLBACK;
+-- Correct identifier, missing file.
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SET TRANSACTION SNAPSHOT 'FFF-FFF-F';
+ROLLBACK;
-- Test for successful cleanup of an aborted transaction at session exit.
-- THIS MUST BE THE LAST TEST IN THIS FILE.
signature.asc
Description: PGP signature
Re: Document that PG_TRY block cannot have a return statement
Hi,
What about this wording:
The code that might throw ereport(ERROR) cannot contain any non local
control flow other than ereport(ERROR) e.g.: return, goto, break, continue.
In other words once PG_TRY() is executed, either PG_CATCH() or PG_FINALLY()
must be executed as well.
I used 'code that might throw ereport(ERROR)' for XXX since this is what's
used earlier in the comment.
On Tue, 12 Sept 2023 at 17:22, Tom Lane wrote:
> Serpent writes:
> > I'm talking about this part:
>
> > PG_TRY();
> > {
> > ... code that might throw ereport(ERROR) ...
> > }
>
> Ah. Your phrasing needs work for clarity then. Also, "return"
> is hardly the only way to break it; break, continue, or goto
> leading out of the PG_TRY are other possibilities. Maybe more
> like "The XXX code must exit normally (by control reaching
> the end) if it does not throw ereport(ERROR)." Not quite sure
> what to use for XXX.
>
> regards, tom lane
>
commit d077d0473378271d8cc4c0685b22ec61c9bce3f4
Author: Serpent7776
Date: Tue Sep 12 14:38:09 2023 +0200
Document that PG_TRY block cannot have a return statement
Document the fact that the block of code that follows `PG_TRY()` cannot
have any form of return statement, because `PG_TRY()` modifies global
state that is later restored in `PG_CATCH()` or `PG_FINALLY()`.
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4a9562fdaa..1bcd146117 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -352,6 +352,11 @@ extern PGDLLIMPORT ErrorContextCallback *error_context_stack;
* You cannot use both PG_CATCH() and PG_FINALLY() in the same
* PG_TRY()/PG_END_TRY() block.
*
+ * The code that might throw ereport(ERROR) cannot contain any non local
+ * control flow other than ereport(ERROR) e.g.: return, goto, break, continue.
+ * In other words once PG_TRY() is executed, either PG_CATCH() or PG_FINALLY()
+ * must be executed as well.
+ *
* Note: while the system will correctly propagate any new ereport(ERROR)
* occurring in the recovery section, there is a small limit on the number
* of levels this will work for. It's best to keep the error recovery
Fixup the variable name to indicate the WAL record reservation status.
Hi All, Please find a small patch to improve code readability by fixing up the variable name to indicate the WAL record reservation status. The insertion is done later in the code based on the reservation status. -- Thanks and Regards, Krishnakumar (KK). [Microsoft] v1-0001-Fixup-the-variable-name-to-indicate-the-wal-recor.patch Description: Binary data
Re: Extract numeric filed in JSONB more effectively
On Thu, Sep 14, 2023 at 5:18 AM Chapman Flack wrote:
> On 2023-09-04 10:35, Andy Fan wrote:
> > v13 attached. Changes includes:
> >
> > 1. fix the bug Jian provides.
> > 2. reduce more code duplication without DirectFunctionCall.
> > 3. add the overlooked jsonb_path_query and jsonb_path_query_first as
> > candidates
>
> Apologies for the delay. I like the way this is shaping up.
This is a great signal:)
>
>
My first comment will be one that may be too large for this patch
> (and too large to rest on my opinion alone); that's why I'm making
> it first.
>
> It seems at first a minor point: to me it feels like a wart to have
> to pass jsonb_finish_numeric (and *only* jsonb_finish_numeric) a type
> oid reflecting the target type of a cast that's going to be applied
> *after jsonb_finish_numeric has done its work*, and only for the
> purpose of generating a message if the jsonb type *isn't numeric*,
> but saying "cannot cast to" (that later target type) instead.
>
> I understand this is done to exactly match the existing behavior,
> so what makes this a larger issue is I'm not convinced the existing
> behavior is good. Therefore I'm not convinced that bending over
> backward to preserve it is good.
>
I hesitated to do so, but I'm thinking if any postgresql user uses
some code like if (errMsg.equals('old-error-message')), and if we
change the error message, the application will be broken. I agree
with the place for the error message, IIUC, you intend to choose
not compatible with the old error message?
What's not good: the places a message from cannotCastJsonbValue
> are produced, there has been no attempt yet to cast anything.
> The message purely tells you about whether you have the kind
> of jsonb node you think you have (and array, bool, null, numeric,
> object, string are the only kinds of those). If you're wrong
> about what kind of jsonb node it is, you get this message.
> If you're right about the kind of node, you don't get this
> message, regardless of whether its value can be cast to the
> later target type. For example, '32768'::jsonb::int2 produces
> ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE "smallint out of range"
> but that message comes from the actual int2 cast.
>
> IMV, what the "cannot cast jsonb foo to type %s" message really
> means is "jsonb foo where jsonb bar is required" and that's what
> it should say, and that message depends on nothing about any
> future plans for what will be done to the jsonb bar, so it can
> be produced without needing any extra information to be passed.
>
> I'm also not convinced ERRCODE_INVALID_PARAMETER_VALUE is a
> good errcode for that message (whatever the wording). I do not
> see much precedent elsewhere in the code for using
> INVALID_PARAMETER_VALUE to signal this kind of "data value
> isn't what you think it is" condition. Mostly it is used
> when checking the kinds of parameters passed to a function to
> indicate what it should do.
>
> There seem to be several more likely choices for an errcode
> there in the 2203x range.
>
> But I understand that issue is not with this patch so much
> as with preexisting behavior, and because it's preexisting,
> there can be sound arguments against changing it.
> But if that preexisting message could be changed, it would
> eliminate the need for an unpleasing wart here.
>
> Other notes are more minor:
>
> + else
> + /* not the desired pattern. */
> + PG_RETURN_POINTER(fexpr);
> ...
> +
> + if (!OidIsValid(new_func_id))
> + PG_RETURN_POINTER(fexpr);
> ...
> + default:
> + PG_RETURN_POINTER(fexpr);
>
> If I am reading supportnodes.h right, returning NULL is
> sufficient to say no transformation is needed.
>
I double confirmed you are right here.
Changed it to PG_RETURN_POINT(null); here in the next version.
>
> + FuncExpr*fexpr = palloc0(sizeof(FuncExpr));
> ...
> + memcpy(fexpr, req->fcall, sizeof(FuncExpr));
>
> Is the shallow copy necessary? If so, a comment explaining why
> might be apropos. Because the copy is shallow, if there is any
> concern about the lifespan of req->fcall, would there not be a
> concern about its children?
>
All the interesting parts in the input FuncExpr are heap based,
but the FuncExpr itself is stack based (I'm not sure why the fact
works like this), Also based on my previously understanding, I
need to return a FuncExpr original even if the function can't be
simplified, so I made a shallow copy. There will be no copy at
all in the following version since I returned NULL in such a case.
> Is there a reason not to transform the _tz flavors of
> jsonb_path_query and jsonb_path-query_first?
>
I misunderstood the _tz flavors return timestamp, after some deep
reading of these functions, they just work at the comparisons part.
so I will add them in the following version.
>
> -
Small patch modifying variable name to reflect the logic involved
Hi All, Please find a small patch to improve code readability by modifying variable name to reflect the logic involved - finding diff between end and start time of WAL sync. -- Thanks and Regards, Krishnakumar (KK) [Microsoft] v1-0001-Improve-code-readability-by-modifying-variable-na.patch Description: Binary data
Re: Infinite Interval
On Wed, Sep 13, 2023 at 6:13 PM Ashutosh Bapat wrote: > > to sum(). I am planning to work on this next week but in case somebody > else wants to pick this up here are patches with other things fixed. > > -- > Best Wishes, > Ashutosh Bapat hi. some doc issues. - decade, century, and millennium). + decade, century, and millennium + for all types and hour and day just for interval). The above part seems not right. some fields do not apply to interval data types. test case: SELECT EXTRACT(epoch FROM interval 'infinity') as epoch ,EXTRACT(YEAR FROM interval 'infinity') as year ,EXTRACT(decade FROM interval 'infinity') as decade ,EXTRACT(century FROM interval 'infinity') as century ,EXTRACT(millennium FROM interval 'infinity') as millennium ,EXTRACT(month FROM interval 'infinity') as mon ,EXTRACT(day FROM interval 'infinity') as day ,EXTRACT(hour FROM interval 'infinity') as hour ,EXTRACT(min FROM interval 'infinity') as min ,EXTRACT(second FROM interval 'infinity') as sec; - date, timestamp + date, timestamp, interval later than all other time stamps it seems we have forgotten to mention the -infinity case, we can fix the doc together, since timestamptz also applies to +/-infinity.
Re: CHECK Constraint Deferrable
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: Cleaning up array_in()
13.09.2023 11:55, jian he wrote:
2) CPPFLAGS="-DARRAYDEBUG" ./configure ... breaks "make check", maybe change
elog(NOTICE) to elog(DEBUG1)?
2a) a message logged there lacks some delimiter before "lBound info":
NOTICE: array_in- ndim 1 ( 3 -1 -1 -1 -1 -1lBound info 1 1 1 1 1 1) for
{red,green,blue}
what about changing the format to "ndim 1 ( 3 -1 -1 -1 -1 -1; lBound info: 1 1 1 1 1
1)"?
fixed. Use elog(DEBUG1) now.
Thanks for the fixes!
I didn't mean to remove the prefix "array_in-", but in fact I was confused
by the "{function_name}-" syntax, and now when I've looked at it closely, I
see that that syntax was quite popular ("date_in- ", "single_decode- ", ...)
back in 1997 (see 9d8ae7977). But nowadays it is out of fashion, with most
of such debugging prints were gone with 7a877dfd2 and the next-to-last one
with 50861cd68. Moreover, as the latter commit shows, such debugging output
can be eliminated completely without remorse. (And I couldn't find mentions
of ARRAYDEBUG in pgsql-bugs, pgsql-hackers archives, so probably no one used
that debugging facility since it's introduction.)
As of now, the output still weird (I mean the excessive right parenthesis):
DEBUG: ndim 1 ( 2 -1 -1 -1 -1 -1); lBound info: 1 1 1 1 1 1) for {0,0}
Otherwise, from a user perspective, the patch set looks good to me. (Though
maybe English language editorialization still needed before committing it.)
Best regards,
Alexander
