Re: [PATCH] add relation and block-level filtering to pg_waldump
On Fri, 25 Feb 2022 at 03:02, David Christensen wrote: > Greetings, > > This patch adds the ability to specify a RelFileNode and optional BlockNum to > limit output of > pg_waldump records to only those which match the given criteria. This should > be more performant > than `pg_waldump | grep` as well as more reliable given specific variations > in output style > depending on how the blocks are specified. > > This currently affects only the main fork, but we could presumably add the > option to filter by fork > as well, if that is considered useful. > Cool. I think we can report an error instead of reading wal files, if the tablespace, database, or relation is invalid. Does there any WAL record that has invalid tablespace, database, or relation OID? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [PATCH] add relation and block-level filtering to pg_waldump
On Fri, 25 Feb 2022 at 20:48, David Christensen wrote: >> Cool. I think we can report an error instead of reading wal files, >> if the tablespace, database, or relation is invalid. Does there any >> WAL record that has invalid tablespace, database, or relation OID? > > The only sort of validity check we could do here is range checking for the > underlying data types > (which we certainly could/should add if it’s known to never be valid for the > underlying types); The invalid OID I said here is such as negative number and zero, for those parameters, we do not need to read the WAL files, since it always invalid. > non-existence of objects is a no-go, since that depends purely on the WAL > range you are > looking at and you’d have to, you know, scan it to see if it existed before > marking as invalid. :) > Agreed. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Size functions inconsistent results
On Fri, 25 Feb 2022 at 22:58, Fabrízio de Royes Mello
wrote:
> Hi all,
>
> While doing some work using our functions [1] for calculate relations size
> I noticed an inconsistency between pg_total_relation_size and calculate
> everything separately, have a look in this example:
>
> fabrizio=# create table test_size (id bigserial primary key, toast_column
> text);
> CREATE TABLE
>
> fabrizio=# insert into test_size (toast_column)
>
> select repeat('X'::text, pg_size_bytes('1MB')::integer)
> from generate_series(1,1000);
> INSERT 0 1000
>
> fabrizio=# with relations as (
> select schemaname, relname, relid
> from pg_stat_user_tables
> where relname = 'test_size'
> ),
> sizes as (
> select
> schemaname,
> r.relname,
>
> pg_total_relation_size(relid) AS total_bytes,
>
> pg_relation_size(relid, 'main') +
> pg_relation_size(relid, 'init') +
> pg_relation_size(relid, 'fsm') +
> pg_relation_size(relid, 'vm') AS heap_bytes,
> pg_indexes_size(relid) AS index_bytes,
> pg_table_size(reltoastrelid) AS toast_bytes
> from relations r
> join pg_class on pg_class.oid = r.relid
> )
> select
> total_bytes, heap_bytes, index_bytes, toast_bytes,
> (total_bytes = (heap_bytes+index_bytes+toast_bytes)) as "Equal?",
> (total_bytes - (heap_bytes+index_bytes+toast_bytes)) as "Diff"
> from sizes;
> total_bytes | heap_bytes | index_bytes | toast_bytes | Equal? | Diff
> -++-+-++
> 14000128 | 90112 | 40960 |13688832 | f | 180224
> (1 row)
>
> I want to calculate separately HEAP, INDEXES and TOAST (including indexes)
> sizes but it seems it's a bit inconsistent with pg_total_relation_size.
>
> Is it correct or am I missing something?
>
I think, you forget the index size of toast table.
with relations as (
select schemaname, relname, relid
from pg_stat_user_tables
where relname = 'test_size'
),
sizes as (
select
schemaname,
r.relname,
pg_total_relation_size(relid) AS total_bytes,
pg_relation_size(relid, 'main') +
pg_relation_size(relid, 'init') +
pg_relation_size(relid, 'fsm') +
pg_relation_size(relid, 'vm') AS heap_bytes,
pg_indexes_size(relid) AS index_bytes,
pg_table_size(reltoastrelid) + pg_indexes_size(reltoastrelid) AS toast_bytes
from relations r
join pg_class on pg_class.oid = r.relid
)
select
total_bytes, heap_bytes, index_bytes, toast_bytes,
(total_bytes = (heap_bytes+index_bytes+toast_bytes)) as "Equal?",
(total_bytes - (heap_bytes+index_bytes+toast_bytes)) as "Diff"
from sizes;
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
CREATE DATABASE IF NOT EXISTS in PostgreSQL
Hi, hackers
When I try to use CREATE DATABASE IF NOT EXISTS in PostgreSQL, it complains
this syntax is not supported. We can use the following command to achieve
this, however, it's not straightforward.
SELECT 'CREATE DATABASE mydb'
WHERE NOT EXISTS (SELECT FROM pg_database WHERE datname = 'mydb')\gexec
Why don't support CREATE DATABASE IF NOT EXISTS syntax in PostgreSQL?
I create a patch for this, any suggestions?
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
>From 7971893868e6eedc7229d28442f07890f251c42b Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Sun, 27 Feb 2022 22:02:59 +0800
Subject: [PATCH v1] Add CREATE DATABASE IF NOT EXISTS syntax
---
doc/src/sgml/ref/create_database.sgml | 2 +-
src/backend/commands/dbcommands.c | 19 +++
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/equalfuncs.c| 1 +
src/backend/parser/gram.y | 9 +
src/include/nodes/parsenodes.h| 1 +
6 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml
index f70d0c75b4..74af9c586e 100644
--- a/doc/src/sgml/ref/create_database.sgml
+++ b/doc/src/sgml/ref/create_database.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
-CREATE DATABASE name
+CREATE DATABASE name [ IF NOT EXISTS ]
[ [ WITH ] [ OWNER [=] user_name ]
[ TEMPLATE [=] template ]
[ ENCODING [=] encoding ]
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index c37e3c9a9a..f3e5e930f9 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -557,10 +557,21 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
* message than "unique index violation". There's a race condition but
* we're willing to accept the less friendly message in that case.
*/
- if (OidIsValid(get_database_oid(dbname, true)))
- ereport(ERROR,
-(errcode(ERRCODE_DUPLICATE_DATABASE),
- errmsg("database \"%s\" already exists", dbname)));
+ {
+ Oid check_dboid = get_database_oid(dbname, true);
+ if (OidIsValid(check_dboid))
+ {
+ if (!stmt->if_not_exists)
+ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_DATABASE),
+ errmsg("database \"%s\" already exists", dbname)));
+
+ ereport(NOTICE,
+ (errcode(ERRCODE_DUPLICATE_DATABASE),
+ errmsg("database \"%s\" already exists, skipping", dbname)));
+ return check_dboid;
+ }
+ }
/*
* The source DB can't have any active backends, except this one
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index d4f8455a2b..83ead22931 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4045,6 +4045,7 @@ _copyCreatedbStmt(const CreatedbStmt *from)
COPY_STRING_FIELD(dbname);
COPY_NODE_FIELD(options);
+ COPY_SCALAR_FIELD(if_not_exists);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index f1002afe7a..f9a89fa4a8 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1722,6 +1722,7 @@ _equalCreatedbStmt(const CreatedbStmt *a, const CreatedbStmt *b)
{
COMPARE_STRING_FIELD(dbname);
COMPARE_NODE_FIELD(options);
+ COMPARE_SCALAR_FIELD(if_not_exists);
return true;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a03b33b53b..72e4f642d9 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10395,6 +10395,15 @@ CreatedbStmt:
CreatedbStmt *n = makeNode(CreatedbStmt);
n->dbname = $3;
n->options = $5;
+ n->if_not_exists = false;
+ $$ = (Node *)n;
+}
+ | CREATE DATABASE IF_P NOT EXISTS name opt_with createdb_opt_list
+{
+ CreatedbStmt *n = makeNode(CreatedbStmt);
+ n->dbname = $6;
+ n->options = $8;
+ n->if_not_exists = true;
$$ = (Node *)n;
}
;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 1617702d9d..71c828ac4d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3295,6 +3295,7 @@ typedef struct CreatedbStmt
NodeTag type;
char *dbname; /* name of database to create */
List *options; /* List of DefElem nodes */
+ boolif_not_exists; /* just do nothing if it already exists? */
} CreatedbStmt;
/* --
--
2.25.1
Re: CREATE DATABASE IF NOT EXISTS in PostgreSQL
On Mon, 28 Feb 2022 at 01:53, Tom Lane wrote: > Japin Li writes: >> Why don't support CREATE DATABASE IF NOT EXISTS syntax in PostgreSQL? > > FWIW, I'm generally hostile to CREATE IF NOT EXISTS semantics across > the board, because of its exceedingly squishy semantics: it ensures > that an object by that name exists, but you have exactly no guarantees > about its properties or contents. Thanks for the explanation! I think it is the database user who should guarantee the properties and contents of a database. CREATE IF NOT EXISTS is just a syntax sugar. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Doc about how to set max_wal_senders when setting minimal wal_level
Hi, hackers When I try to change wal_level to minimal and restart the database, it complains max_wal_senders > 0. 2022-03-03 10:10:16.938 CST [6389] FATAL: WAL streaming (max_wal_senders > 0) requires wal_level "replica" or "logical" However, the documentation about wal_level [1] doesn't mentation this. How about adding a sentence to describe how to set max_wal_senders when setting wal_level to minimal? [1] https://www.postgresql.org/docs/devel/runtime-config-wal.html -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd. diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 7ed8c82a9d..3f5ecc4a04 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -2776,6 +2776,10 @@ include_dir 'conf.d' unavailable for archive recovery and standby server, which may lead to data loss. + +When changing wal_level to minimal, +you must set the parameter to 0. + In logical level, the same information is logged as with replica, plus information needed to allow
Re: Doc about how to set max_wal_senders when setting minimal wal_level
On Thu, 03 Mar 2022 at 11:25, David G. Johnston wrote: > I would suggest a wording more like: > > "A precondition for using minimal WAL is to disable WAL archiving and > streaming replication by setting max_wal_senders to 0, and archive_mode to > off." > > While accurate, the phrase "you must set" just doesn't feel right to me. I > also don't like how the proposed sentence (either one) is added separately > as opposed to being included in the immediately preceding paragraph. While > this limited patch is probably sufficient I would suggest trying to work > out a slightly larger patch the improves the wording on the entire existing > paragraph while incorporating the reference to max_wal_senders. > Thanks for your review. Modified as your suggestion. > Note, we seem to be missing the documentation of the default setting for > archive_mode. > Add the default value for archive_mode. > In addition, max_wal_senders could also be changed, adding a sentence like: > > "If setting max_wal_senders to 0 consider also reducing the amount of WAL > produced by changing wal_level to minimal." > Modified. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd. diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 7ed8c82a9d..4fd36fa53a 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -2775,6 +2775,10 @@ include_dir 'conf.d' minimal makes any base backups taken before unavailable for archive recovery and standby server, which may lead to data loss. +A precondition for using minimal WAL is to disable WAL archiving and +streaming replication by setting +to 0, and archive_mode +to off. In logical level, the same information is logged as @@ -3535,6 +3539,7 @@ include_dir 'conf.d' mode. In always mode, all files restored from the archive or streamed with streaming replication will be archived (again). See for details. +The default value is off. This parameter can only be set at server start. @@ -4096,7 +4101,9 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows reconnect. This parameter can only be set at server start. Also, wal_level must be set to replica or higher to allow connections from standby -servers. +servers. If setting max_wal_senders to +0 consider also reducing the amount of WAL produced +by changing wal_level to minimal.
Re: Doc about how to set max_wal_senders when setting minimal wal_level
On Thu, 03 Mar 2022 at 12:10, Japin Li wrote: > On Thu, 03 Mar 2022 at 11:25, David G. Johnston > wrote: >> I would suggest a wording more like: >> >> "A precondition for using minimal WAL is to disable WAL archiving and >> streaming replication by setting max_wal_senders to 0, and archive_mode to >> off." >> >> While accurate, the phrase "you must set" just doesn't feel right to me. I >> also don't like how the proposed sentence (either one) is added separately >> as opposed to being included in the immediately preceding paragraph. While >> this limited patch is probably sufficient I would suggest trying to work >> out a slightly larger patch the improves the wording on the entire existing >> paragraph while incorporating the reference to max_wal_senders. >> > > Thanks for your review. Modified as your suggestion. > >> Note, we seem to be missing the documentation of the default setting for >> archive_mode. >> > > Add the default value for archive_mode. > >> In addition, max_wal_senders could also be changed, adding a sentence like: >> >> "If setting max_wal_senders to 0 consider also reducing the amount of WAL >> produced by changing wal_level to minimal." >> > > Modified. Attach v3 patch to fix missing close varname tag. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd. diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 7ed8c82a9d..bc7ba1e120 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -2775,6 +2775,10 @@ include_dir 'conf.d' minimal makes any base backups taken before unavailable for archive recovery and standby server, which may lead to data loss. +A precondition for using minimal WAL is to disable WAL archiving and +streaming replication by setting +to 0, and archive_mode +to off. In logical level, the same information is logged as @@ -3535,6 +3539,7 @@ include_dir 'conf.d' mode. In always mode, all files restored from the archive or streamed with streaming replication will be archived (again). See for details. +The default value is off. This parameter can only be set at server start. @@ -4096,7 +4101,9 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows reconnect. This parameter can only be set at server start. Also, wal_level must be set to replica or higher to allow connections from standby -servers. +servers. If setting max_wal_senders to +0 consider also reducing the amount of WAL produced +by changing wal_level to minimal.
Re: Doc about how to set max_wal_senders when setting minimal wal_level
On Fri, 04 Mar 2022 at 14:05, Kyotaro Horiguchi wrote: > At Fri, 04 Mar 2022 12:18:29 +0800, Japin Li wrote in >> >> On Thu, 03 Mar 2022 at 12:10, Japin Li wrote: >> >> Attach v3 patch to fix missing close varname tag. > > +A precondition for using minimal WAL is to disable WAL archiving and > +streaming replication by setting linkend="guc-max-wal-senders"/> > +to 0, and archive_mode > +to off. > > It is a bit odd that the features to stop and the corresponding GUCs > are written irrespectively. It would be better they're in the same > order. > Thanks for your review. Modified. > +servers. If setting max_wal_senders to > +0 consider also reducing the amount of WAL > produced > +by changing wal_level to > minimal. > > Those who anively follow this suggestion may bump into failure when > arhive_mode is on. Thus archive_mode is also worth referred to. But, > anyway, IMHO, it is mere a performance tips that is not necessarily > required in this section, or even in this documentaiotn. Addtion to > that, if we write this for max_wal_senders, archive_mode will deserve > the similar tips but I think it is too verbose. In short, I think I > would not add that description at all. > It already has a tip about wal_level for archive_mode [1], IIUC. archive_mode cannot be enabled when wal_level is set to minimal. [1] https://www.postgresql.org/docs/devel/runtime-config-wal.html#GUC-ARCHIVE-MODE -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd. diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 7ed8c82a9d..a70adb7030 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -2775,6 +2775,10 @@ include_dir 'conf.d' minimal makes any base backups taken before unavailable for archive recovery and standby server, which may lead to data loss. +A precondition for using minimal WAL is to disable WAL archiving and +streaming replication by setting archive_mode to +off, and to +0. In logical level, the same information is logged as @@ -3535,6 +3539,7 @@ include_dir 'conf.d' mode. In always mode, all files restored from the archive or streamed with streaming replication will be archived (again). See for details. +The default value is off. This parameter can only be set at server start. @@ -4096,7 +4101,9 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows reconnect. This parameter can only be set at server start. Also, wal_level must be set to replica or higher to allow connections from standby -servers. +servers. If setting max_wal_senders to +0 consider also reducing the amount of WAL produced +by changing wal_level to minimal.
Re: support for MERGE
On Sat, 12 Mar 2022 at 11:53, Alvaro Herrera wrote:
> On 2022-Mar-09, Zhihong Yu wrote:
>
>> Hi,
>> For v14-0002-Split-ExecUpdate-and-ExecDelete-in-reusable-piec.patch :
>>
>> + TupleTableSlot *insertProjectedTuple;
>>
>> With `insert` at the beginning of the variable name, someone may think it
>> is a verb. How about naming the variable projectedTupleFromInsert (or
>> something similar) ?
>
> I went with projectedFromInsert. I don't feel a need to call it a
> "tuple" because it's already a TupleTableSlot * ...
>
>> + if (!ExecBRDeleteTriggers(context->estate, context->epqstate,
>> + resultRelInfo, tupleid, oldtuple,
>> + epqreturnslot))
>> + /* some trigger made the delete a no-op; let caller know */
>> + return false;
>>
>> It seems you can directly return the value returned
>> from ExecBRDeleteTriggers().
>
> True, let's do that.
>
> On 2022-Mar-10, Simon Riggs wrote:
>
>> Various cases were not tested in the patch - additional patch
>> attached, but nothing surprising there.
>
> Thanks, incorporated here.
>
> This is mostly just a rebase to keep cfbot happy.
Hi, hackers
+ ar_delete_trig_tcs = mtstate->mt_transition_capture;
+ if (mtstate->operation == CMD_UPDATE && mtstate->mt_transition_capture
&&
+ mtstate->mt_transition_capture->tcs_update_old_table)
+ {
+ ExecARUpdateTriggers(estate, resultRelInfo, tupleid, oldtuple,
+NULL, NULL,
mtstate->mt_transition_capture);
+
+ /*
+* We've already captured the NEW TABLE row, so make sure any AR
+* DELETE trigger fired below doesn't capture it again.
+*/
+ ar_delete_trig_tcs = NULL;
+ }
Maybe we can use ar_delete_trig_tcs in if condition and ExecARUpdateTriggers()
to make the code shorter.
+ /* "old" transition table for DELETE, if any */
+ Tuplestorestate *old_del_tuplestore;
+ /* "new" transition table INSERT, if any */
+ Tuplestorestate *new_ins_tuplestore;
Should we add "for" for transition INSERT comment?
+MERGE is a multiple-table, multiple-action command: it specifies a target
+table and a source relation, and can contain multiple WHEN MATCHED and
+WHEN NOT MATCHED clauses, each of which specifies one UPDATE, INSERT,
+UPDATE, or DO NOTHING actions. The target table is modified by MERGE,
+and the source relation supplies additional data for the actions
I think the "source relation" is inaccurate. How about using "data source"
like document?
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: Support logical replication of DDLs
Hi, Zhang Li On Thu, 17 Mar 2022 at 05:17, Zheng Li wrote: > Hi, > >>If you don't mind, would you like to share the POC or the branch for this >>work? > > The POC patch is attached. It currently supports the following > functionalities: > 1. Configure either database level or table level DDL replication via > the CREATE PUBLICATION command. > > 2.Supports replication of DDL of the following types when database > level DDL replication is turned on. Other less common DDL types could > be added later. > T_CreateSchemaStmt > T_CreateStmt > T_CreateForeignTableStmt > T_AlterDomainStmt > T_DefineStmt > T_CompositeTypeStmt > T_CreateEnumStmt > T_CreateRangeStmt > T_AlterEnumStmt > T_ViewStmt > T_CreateFunctionStmt > T_AlterFunctionStmt > T_CreateTrigStmt > T_CreateDomainStmt > T_CreateCastStmt > T_CreateOpClassStmt > T_CreateOpFamilyStmt > T_AlterOpFamilyStmt > T_AlterOperatorStmt > T_AlterTypeStmt > T_GrantStmt > T_AlterCollationStmt > T_AlterTableStmt > T_IndexStmt > > 3.Supports replication of DDLs of the following types if only table > level DDL replication is turned on. > T_AlterTableStmt > T_IndexStmt > > 4.Supports seamless DML replication of new tables created via DDL > replication without having to manually running “ALTER SUBSCRIPTION ... > REFRESH PUBLICATION" on the subscriber. > Great! I think we can split the patch into several pieces (at least implementation and test cases) for easier review. + * Simiarl to the generic logical messages, These DDL messages can be either + * transactional or non-transactional. Note by default DDLs in PostgreSQL are + * transactional. There is a typo, s/Simiarl/Similar/. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: support for MERGE
On Thu, 17 Mar 2022 at 03:18, Alvaro Herrera wrote:
> v16.
> On 2022-Mar-14, Japin Li wrote:
>
>> + ar_delete_trig_tcs = mtstate->mt_transition_capture;
>> + if (mtstate->operation == CMD_UPDATE &&
>> mtstate->mt_transition_capture &&
>> + mtstate->mt_transition_capture->tcs_update_old_table)
>> + {
>> + ExecARUpdateTriggers(estate, resultRelInfo, tupleid,
>> oldtuple,
>> +NULL, NULL,
>> mtstate->mt_transition_capture);
>> +
>> + /*
>> +* We've already captured the NEW TABLE row, so make sure
>> any AR
>> +* DELETE trigger fired below doesn't capture it again.
>> +*/
>> + ar_delete_trig_tcs = NULL;
>> + }
>>
>> Maybe we can use ar_delete_trig_tcs in if condition and
>> ExecARUpdateTriggers() to make the code shorter.
>
> ... Well, the code inside the block is about UPDATE triggers, so it's a
> nasty to use the variable whose name says it's for DELETE triggers.
> That variable really is just a clever flag that indicates whether or not
> to call the DELETE part. It's a bit of strange coding, but ...
>
>> +MERGE is a multiple-table, multiple-action command: it specifies a target
>> +table and a source relation, and can contain multiple WHEN MATCHED and
>> +WHEN NOT MATCHED clauses, each of which specifies one UPDATE, INSERT,
>> +UPDATE, or DO NOTHING actions. The target table is modified by MERGE,
>> +and the source relation supplies additional data for the actions
>>
>> I think the "source relation" is inaccurate. How about using "data
>> source" like document?
>
> I debated this with myself when I started using the term "source
> relation" here. The result of a query is also a relation, so this term
> is not incorrect; in fact, the glossary entry for "relation" explains
> this:
> https://www.postgresql.org/docs/14/glossary.html#GLOSSARY-RELATION
>
> It's true that it's not the typical use of the term in our codebase.
>
> Overall, I'm -0 for changing this. (If we decide to change it, there
> are other places that would need to change as well.)
Thanks for the detailed explanation.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Thu, 17 Mar 2022 at 21:31, Alexander Korotkov wrote:
> On Thu, Mar 17, 2022 at 4:23 PM Peter Eisentraut
> wrote:
>> On 17.03.22 14:12, Aleksander Alekseev wrote:
>> > v19-0001 changes the format string for XIDs from %u to XID_FMT. This
>> > refactoring allows us to switch to UINT64_FORMAT by changing one
>> > #define in the future patches.
>> >
>> > Kyotaro suggested using `errmsg("blah blah %lld ..", (long long)
>> > xid)` instead in order to simplify localization of the error messages.
>> > Personally I don't have a strong opinion here. Either approach will
>> > work and will affect the error messages eventually. Please let us know
>> > what you think.
>>
>> This is not a question of simplification. Translatable messages with
>> embedded macros won't work. This patch isn't going to be acceptable.
>
> I've suspected this, but wasn't sure. Thank you for clarification.
>
Maybe, we should format it to string before for localization messages,
like the following code snippet comes from pg_backup_tar.c.
However, I do not think it is a good way.
snprintf(buf1, sizeof(buf1), INT64_FORMAT, (int64) len);
snprintf(buf2, sizeof(buf2), INT64_FORMAT, (int64) th->fileLen);
fatal("actual file length (%s) does not match expected (%s)",
buf1, buf2);
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: Support logical replication of DDLs
On Thu, 17 Mar 2022 at 05:17, Zheng Li wrote: > Hi, > >>If you don't mind, would you like to share the POC or the branch for this >>work? > > The POC patch is attached. It currently supports the following > functionalities: Hi, When I try to run regression test, there has some errors. make[2]: Entering directory '/home/px/Codes/postgresql/Debug/src/test/subscription' rm -rf '/home/px/Codes/postgresql/Debug/src/test/subscription'/tmp_check && /usr/bin/mkdir -p '/home/px/Codes/postgresql/Debug/src/test/subscription'/tmp_check && cd /home/px/Codes/postgresql/Debug/../src /test/subscription && TESTDIR='/home/px/Codes/postgresql/Debug/src/test/subscription' PATH="/home/px/Codes/postgresql/Debug/tmp_install/home/px/Codes/postgresql/Debug/pg/bin:/home/px/Codes/postgresql/Debu g/src/test/subscription:$PATH" LD_LIBRARY_PATH="/home/px/Codes/postgresql/Debug/tmp_install/home/px/Codes/postgresql/Debug/pg/lib:$LD_LIBRARY_PATH" PGPORT='65432' PG_REGRESS='/home/px/Codes/postgresql/De bug/src/test/subscription/../../../src/test/regress/pg_regress' /usr/bin/prove -I /home/px/Codes/postgresql/Debug/../src/test/perl/ -I /home/px/Codes/postgresql/Debug/../src/test/subscription t/*.pl t/001_rep_changes.pl ... ok t/002_types.pl . ok t/003_constraints.pl ... ok t/004_sync.pl .. 6/? # Tests were run but no plan was declared and done_testing() was not seen. # Looks like your test exited with 29 just after 7. t/004_sync.pl .. Dubious, test returned 29 (wstat 7424, 0x1d00) All 7 subtests passed t/005_encoding.pl .. ok t/006_rewrite.pl ... 1/? # poll_query_until timed out executing this query: # SELECT '0/14A8648' <= replay_lsn AND state = 'streaming' FROM pg_catalog.pg_stat_replication WHERE application_name = 'mysub'; # expecting this output: # t # last actual query output: # # with stderr: # Tests were run but no plan was declared and done_testing() was not seen. # Looks like your test exited with 29 just after 1. t/006_rewrite.pl ... Dubious, test returned 29 (wstat 7424, 0x1d00) All 1 subtests passed t/007_ddl.pl ... ok t/008_diff_schema.pl ... 1/? # Tests were run but no plan was declared and done_testing() was not seen. # Looks like your test exited with 29 just after 4. t/008_diff_schema.pl ... Dubious, test returned 29 (wstat 7424, 0x1d00) All 4 subtests passed t/009_matviews.pl .. Dubious, test returned 29 (wstat 7424, 0x1d00) No subtests run The new patch change the behavior of publication, when we drop a table on publisher, the table also be dropped on subscriber, and this made the regression testa failed, since the regression test will try to drop the table on subscriber. IMO, we can disable the DDLs replication by default, which makes the regression test happy. Any thoughts? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Support logical replication of DDLs
On Fri, 18 Mar 2022 at 00:22, Zheng Li wrote:
> Hello Japin,
>>The new patch change the behavior of publication, when we drop a table
>>on publisher, the table also be dropped on subscriber, and this made the
>>regression testa failed, since the regression test will try to drop the
>>table on subscriber.
>
>>IMO, we can disable the DDLs replication by default, which makes the
>>regression test happy. Any thoughts?
>
> Good catch, I forgot to run the whole TAP test suite. I think for now
> we can simply disable DDL replication for the failing tests when
> creating publication: $node_publisher->safe_psql('postgres',
> "CREATE PUBLICATION tap_pub FOR ALL TABLES WITH (ddl='')");
> I'll fix it in the next version of patch. I'll also work on breaking
> down the patch into smaller pieces for ease of review.
>
Oh, it doesn't work.
postgres=# CREATE PUBLICATION s1 FOR ALL TABLES WITH(ddl = '');;
ERROR: conflicting or redundant options
Here is the code, I think, you might mean `if (ddl_level_given == NULL)`.
+ if (*ddl_level_given)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("conflicting or
redundant options")));
+
+ /*
+* If publish option was given only the explicitly
listed actions
+* should be published.
+*/
+ pubactions->pubddl_database = false;
+ pubactions->pubddl_table = false;
+
+ *ddl_level_given = true;
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: Support logical replication of DDLs
On Fri, 18 Mar 2022 at 08:18, Zheng Li wrote: > Hello, > > Attached please find the broken down patch set. Also fixed the failing > TAP tests Japin reported. > Thanks for updating the patchset, I will try it later. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Support logical replication of DDLs
On Fri, 18 Mar 2022 at 08:22, Japin Li wrote:
> On Fri, 18 Mar 2022 at 00:22, Zheng Li wrote:
>> Hello Japin,
>>>The new patch change the behavior of publication, when we drop a table
>>>on publisher, the table also be dropped on subscriber, and this made the
>>>regression testa failed, since the regression test will try to drop the
>>>table on subscriber.
>>
>>>IMO, we can disable the DDLs replication by default, which makes the
>>>regression test happy. Any thoughts?
>>
>> Good catch, I forgot to run the whole TAP test suite. I think for now
>> we can simply disable DDL replication for the failing tests when
>> creating publication: $node_publisher->safe_psql('postgres',
>> "CREATE PUBLICATION tap_pub FOR ALL TABLES WITH (ddl='')");
>> I'll fix it in the next version of patch. I'll also work on breaking
>> down the patch into smaller pieces for ease of review.
>>
>
> Oh, it doesn't work.
>
> postgres=# CREATE PUBLICATION s1 FOR ALL TABLES WITH(ddl = '');;
> ERROR: conflicting or redundant options
>
>
> Here is the code, I think, you might mean `if (ddl_level_given == NULL)`.
>
> + if (*ddl_level_given)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("conflicting or
> redundant options")));
> +
> + /*
> + * If publish option was given only the explicitly
> listed actions
> + * should be published.
> + */
> + pubactions->pubddl_database = false;
> + pubactions->pubddl_table = false;
> +
> + *ddl_level_given = true;
Oh, Sorry, I misunderstand it, it just like you forget initialize
*ddl_level_given to false when enter parse_publication_options().
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: Support logical replication of DDLs
84)
Failed tests: 136-139, 362-365, 588-591, 1040-1043, 1492-1495
1719-1722, 1946-1949, 2177-2180, 2407-2410
2633-2636, 2859-2862, 3085-3088, 3537-3540
3763-3766, 3989-3992, 4215-4218, 4441-
5119-5122, 5345-5348, 6702-6705, 7154-7157
Non-zero exit status: 84
Files=4, Tests=7808, 26 wallclock secs ( 0.46 usr 0.05 sys + 7.98 cusr 2.80
csys = 11.29 CPU)
Result: FAIL
make[2]: *** [Makefile:55: check] Error 1
make[2]: Leaving directory '/home/px/Codes/postgresql/Debug/src/bin/pg_dump'
make[1]: *** [Makefile:43: check-pg_dump-recurse] Error 2
make[1]: Leaving directory '/home/px/Codes/postgresql/Debug/src/bin'
make: *** [GNUmakefile:71: check-world-src/bin-recurse] Error 2
And, when I try to use git am to apply the patch, it complains:
$ git am ~/0001-syntax-pg_publication-pg_dump-ddl_replication.patch
Patch format detection failed.
[1]
https://www.postgresql.org/message-id/MEYP282MB1669DDF788C623B7F8B64C2EB6139%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Remove INT64_FORMAT in translatable strings
Hi,
I found the following lines in pg_backup_tar.c.
if (len != th->fileLen)
{
charbuf1[32],
buf2[32];
snprintf(buf1, sizeof(buf1), INT64_FORMAT, (int64) len);
snprintf(buf2, sizeof(buf2), INT64_FORMAT, (int64) th->fileLen);
fatal("actual file length (%s) does not match expected (%s)",
buf1, buf2);
}
we can rely on %lld/%llu and we decided to use them in translatable strings.
See 6a1cd8b.
However, I am not sure how to update the *.po files under the pg_dump/po
directory. Any suggestions?
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
>From 29bfcdf96324433cd8f4beada7023a27fc2bf0b8 Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Fri, 18 Mar 2022 23:09:22 +0800
Subject: [PATCH v1] Remove use of [U]INT64_FORMAT in some translatable strings
This is similar to 62aa2bb and 6a1cd8b.
---
src/bin/pg_dump/pg_backup_tar.c | 48 +++--
1 file changed, 9 insertions(+), 39 deletions(-)
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index 5c351acda0..b5e26a4b6f 100644
--- a/src/bin/pg_dump/pg_backup_tar.c
+++ b/src/bin/pg_dump/pg_backup_tar.c
@@ -1102,15 +1102,8 @@ _tarAddFile(ArchiveHandle *AH, TAR_MEMBER *th)
fatal("could not close temporary file: %m");
if (len != th->fileLen)
- {
- char buf1[32],
- buf2[32];
-
- snprintf(buf1, sizeof(buf1), INT64_FORMAT, (int64) len);
- snprintf(buf2, sizeof(buf2), INT64_FORMAT, (int64) th->fileLen);
- fatal("actual file length (%s) does not match expected (%s)",
- buf1, buf2);
- }
+ fatal("actual file length (%lld) does not match expected (%lld)",
+ (long long int) len, (long long int) th->fileLen);
pad = tarPaddingBytesRequired(len);
for (i = 0; i < pad; i++)
@@ -1140,24 +1133,14 @@ _tarPositionTo(ArchiveHandle *AH, const char *filename)
/* Go to end of current file, if any */
if (ctx->tarFHpos != 0)
{
- char buf1[100],
- buf2[100];
-
- snprintf(buf1, sizeof(buf1), INT64_FORMAT, (int64) ctx->tarFHpos);
- snprintf(buf2, sizeof(buf2), INT64_FORMAT, (int64) ctx->tarNextMember);
- pg_log_debug("moving from position %s to next member at file position %s",
- buf1, buf2);
+ pg_log_debug("moving from position %lld to next member at file position %lld",
+ (long long int) ctx->tarFHpos, (long long int) ctx->tarNextMember);
while (ctx->tarFHpos < ctx->tarNextMember)
_tarReadRaw(AH, &c, 1, NULL, ctx->tarFH);
}
- {
- char buf[100];
-
- snprintf(buf, sizeof(buf), INT64_FORMAT, (int64) ctx->tarFHpos);
- pg_log_debug("now at file position %s", buf);
- }
+ pg_log_debug("now at file position %lld", (long long int) ctx->tarFHpos);
/* We are at the start of the file, or at the next member */
@@ -1265,25 +1248,12 @@ _tarGetHeader(ArchiveHandle *AH, TAR_MEMBER *th)
len = read_tar_number(&h[124], 12);
- {
- char posbuf[32];
- char lenbuf[32];
-
- snprintf(posbuf, sizeof(posbuf), UINT64_FORMAT, (uint64) hPos);
- snprintf(lenbuf, sizeof(lenbuf), UINT64_FORMAT, (uint64) len);
- pg_log_debug("TOC Entry %s at %s (length %s, checksum %d)",
- tag, posbuf, lenbuf, sum);
- }
+ pg_log_debug("TOC Entry %s at %llu (length %llu, checksum %d)",
+ tag, (unsigned long long) hPos, (unsigned long long) len, sum);
if (chk != sum)
- {
- char posbuf[32];
-
- snprintf(posbuf, sizeof(posbuf), UINT64_FORMAT,
- (uint64) ftello(ctx->tarFH));
- fatal("corrupt tar header found in %s (expected %d, computed %d) file position %s",
- tag, sum, chk, posbuf);
- }
+ fatal("corrupt tar header found in %s (expected %d, computed %d) file position %llu",
+ tag, sum, chk, (unsigned long long) ftello(ctx->tarFH));
th->targetFile = pg_strdup(tag);
th->fileLen = len;
--
2.25.1
Re: Support logical replication of DDLs
On Sat, 19 Mar 2022 at 01:25, Zheng Li wrote: > Hello, > > Thanks for the quick review! > >> And, when I try to use git am to apply the patch, it complains: >> >> $ git am ~/0001-syntax-pg_publication-pg_dump-ddl_replication.patch >> Patch format detection failed. > > git apply works for me. I'm not sure why git am complains. > I also created a git branch to make code sharing easier, please try this out: > https://github.com/zli236/postgres/tree/ddl_replication > Yeah, I can use git apply, I'm not sure how did you generate the patch. >> I also think that ddl = '' isn't a good way to disable DDL replication, how >> about using ddl = 'none' or something else? > > The syntax follows the existing convention of the WITH publish option. > For example in CREATE PUBLICATION mypub WITH (publish = '') > it means don't publish any DML change. So I'd leave it as is for now. > Agreed. >> The test_decoding test case cannot work as expected, see below: > . >> DDL message: transactional: 1 prefix: role: redacted, search_path: >> "$user", public, sz: 47 content:CREATE TABLE tab1 (id serial unique, data >> int); >> BEGIN >> sequence public.tab1_id_seq: transactional:1 last_value: 1 log_cnt: 0 >> is_called:0 >> >> Since the DDL message contains the username, and we try to replace the >> username with 'redacted' to avoid this problem, >> >> \o | sed 's/role.*search_path/role: redacted, search_path/g' >> >> However, the title and dash lines may have different lengths for different >> usernames. To avoid this problem, how about using a specified username when >> initializing the database for this regression test? > > I don't understand the question, do you have an example of when the > test doesn't work? It runs fine for me. > You should use a different user that has different length from your current one. For example: px@localhost$ make check-world > >> t/002_pg_dump.pl .. 13/? >> # Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub1' >> # at t/002_pg_dump.pl line 3916. >> # Review binary_upgrade results in >> /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI > .. >> Failed 84/7709 subtests >> t/003_pg_dump_with_server.pl .. ok >> t/010_dump_connstr.pl . ok >> >> Test Summary Report >> --- >> t/002_pg_dump.pl(Wstat: 21504 Tests: 7709 Failed: 84) > > This is fixed in the latest version. I need to remind myself to run > make check-world in the future. > Thanks for updating the patch. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Remove INT64_FORMAT in translatable strings
On Sat, 19 Mar 2022 at 01:12, Tom Lane wrote: > Japin Li writes: >> we can rely on %lld/%llu and we decided to use them in translatable strings. > > Seems like good cleanup, so pushed. I think though that project style > is to use "long long" or "unsigned long long", without the unnecessary > "int" --- it certainly makes little sense to do it both ways in the > same patch. > >> However, I am not sure how to update the *.po files under the pg_dump/po >> directory. Any suggestions? > > The translation team manages those files. We don't normally touch them > during code development. > Thank you for pushing the patch. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [PATCH] Remove workarounds to format [u]int64's
On Mon, 21 Mar 2022 at 17:23, Aleksander Alekseev
wrote:
> Hi Pavel,
>
>> Probably you can do (long long) instead of (long long int). It is shorter
>> and this is used elsewhere in the code.
>
> Thanks! Here is the updated patch. I also added Reviewed-by: and
> Discussion: to the commit message.
Hi,
After apply the patch, I found pg_checksums.c also has the similar code.
In progress_report(), I'm not sure we can do this replace for this code.
snprintf(total_size_str, sizeof(total_size_str), INT64_FORMAT,
total_size / (1024 * 1024));
snprintf(current_size_str, sizeof(current_size_str), INT64_FORMAT,
current_size / (1024 * 1024));
fprintf(stderr, _("%*s/%s MB (%d%%) computed"),
(int) strlen(current_size_str), current_size_str, total_size_str,
percent);
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 7e69475947..e45e436683 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -657,11 +657,11 @@ main(int argc, char *argv[])
progress_report(true);
printf(_("Checksum operation completed\n"));
- printf(_("Files scanned: %s\n"), psprintf(INT64_FORMAT, files_scanned));
- printf(_("Blocks scanned: %s\n"), psprintf(INT64_FORMAT, blocks_scanned));
+ printf(_("Files scanned: %lld\n"), files_scanned);
+ printf(_("Blocks scanned: %lld\n"), blocks_scanned);
if (mode == PG_MODE_CHECK)
{
- printf(_("Bad checksums: %s\n"), psprintf(INT64_FORMAT, badblocks));
+ printf(_("Bad checksums: %lld\n"), badblocks);
printf(_("Data checksum version: %u\n"), ControlFile->data_checksum_version);
if (badblocks > 0)
@@ -669,8 +669,8 @@ main(int argc, char *argv[])
}
else if (mode == PG_MODE_ENABLE)
{
- printf(_("Files written: %s\n"), psprintf(INT64_FORMAT, files_written));
- printf(_("Blocks written: %s\n"), psprintf(INT64_FORMAT, blocks_written));
+ printf(_("Files written: %lld\n"), files_written);
+ printf(_("Blocks written: %lld\n"), blocks_written);
}
}
Re: [PATCH] Add reloption for views to enable RLS
On Mon, 21 Mar 2022 at 20:40, Laurenz Albe wrote:
> On Mon, 2022-03-21 at 18:09 +0800, Japin Li wrote:
>> After apply the patch, I found pg_checksums.c also has the similar code.
>>
>> In progress_report(), I'm not sure we can do this replace for this code.
>>
>> snprintf(total_size_str, sizeof(total_size_str), INT64_FORMAT,
>> total_size / (1024 * 1024));
>> snprintf(current_size_str, sizeof(current_size_str), INT64_FORMAT,
>> current_size / (1024 * 1024));
>>
>> fprintf(stderr, _("%*s/%s MB (%d%%) computed"),
>> (int) strlen(current_size_str), current_size_str, total_size_str,
>> percent);
>
> I think you replied to the wrong thread...
>
I'm sorry! There is a problem with my email client and I didn't notice the
subject of the reply email.
Again, sorry for the noise!
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Wed, 23 Mar 2022 at 01:22, Maxim Orlov wrote:
> Hi!
>
> Here is v24. Changes are:
> - correct commit messages for 0001 and 0002
> - use uint64 for SLRU page numbering instead of int64 in v23
> - fix code formatting and indent
> - and minor fixes in slru.c
>
> Reviews are very welcome!
>
Thanks for updating the patchs. I have a little comment on 0002.
+errmsg_internal("found xmax %llu" " (infomask
0x%04x) not frozen, not multi, not normal",
+(unsigned long
long) xid, tuple->t_infomask)));
IMO, we can remove the double quote inside the sentence.
errmsg_internal("found xmax %llu (infomask
0x%04x) not frozen, not multi, not normal",
(unsigned long
long) xid, tuple->t_infomask)));
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW
On Wed, 03 Mar 2021 at 20:56, David Steele wrote: > Do you know if you will have time to review this patch during the > current commitfest? > Sorry for the late reply! I think I have time to review this patch and I will do it later. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW
On Thu, 04 Mar 2021 at 14:53, Bharath Rupireddy
wrote:
> Thanks! I will look forward for more review comments.
>
v4-0001-Rearrange-Refresh-Mat-View-Code.patch
-
+static Oid
+get_new_heap_oid(RefreshMatViewStmt *stmt, Relation matviewRel, Oid matviewOid,
+char *relpersistence)
+{
+ Oid OIDNewHeap;
+ boolconcurrent;
+ Oid tableSpace;
+
+ concurrent = stmt->concurrent;
+
+ /* Concurrent refresh builds new data in temp tablespace, and does
diff. */
+ if (concurrent)
+ {
+ tableSpace = GetDefaultTablespace(RELPERSISTENCE_TEMP, false);
+ *relpersistence = RELPERSISTENCE_TEMP;
+ }
Since the concurrent only use in one place, I think we can remove the local
variable
concurrent in get_new_heap_oid().
The others looks good to me.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: Inquiries about PostgreSQL's system catalog development——from a student developer of Nanjing University
On Sat, 06 Mar 2021 at 17:01, 杨逸存 <1057206...@qq.com> wrote: > Dear hacker: > I am a Nanjing University student, Yang. I have forked a newly > version of PostgreSQL source code to develop for my own use. Her is my > question: I am trying to add a new system catalog to the system backend, how > can I reach it? Is there any code or interface demonstration to show me? > I am looking forward to your prompt reply. Heartfelt thanks. Here is a document on how to create a new system catalog for PostgreSQL 11. https://blog.japinli.top/2019/08/postgresql-new-catalog/ -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW
On Fri, 05 Mar 2021 at 19:48, Bharath Rupireddy wrote: > Attaching v5 patch set for further review. > The v5 patch looks good to me, if there is no objection, I'll change the cf status to "Ready for Committer" in few days. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW
On Sun, 07 Mar 2021 at 14:25, Bharath Rupireddy wrote: > On Sun, Mar 7, 2021 at 11:49 AM Japin Li wrote: >> >> On Fri, 05 Mar 2021 at 19:48, Bharath Rupireddy >> wrote: >> > Attaching v5 patch set for further review. >> > >> >> The v5 patch looks good to me, if there is no objection, I'll change the >> cf status to "Ready for Committer" in few days. > > Thanks for the review. > > As I mentioned upthread, I have 2 open points: > 1) In the patch I have added a new mat view info parameter to > ExplainOneQuery(), do we also need to add it to > ExplainOneQuery_hook_type? IMO, we should not (for now), because this > would create a backward compatibility issue. Sorry, I do not know how PostgreSQL handle the backward compatibility issue. Is there a guideline? > 2) Do we document (under respective command pages or somewhere else) > that we allow explain/explain analyze for a command? > IMO, we can add a new page to list the commands that can be explain/explain analyze, since it's clear for users. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW
On Sun, 07 Mar 2021 at 17:33, Bharath Rupireddy wrote: > On Sun, Mar 7, 2021 at 12:13 PM Japin Li wrote: >> >> On Sun, 07 Mar 2021 at 14:25, Bharath Rupireddy >> wrote: >> > On Sun, Mar 7, 2021 at 11:49 AM Japin Li wrote: >> >> >> >> On Fri, 05 Mar 2021 at 19:48, Bharath Rupireddy >> >> wrote: >> >> > Attaching v5 patch set for further review. >> >> > >> >> >> >> The v5 patch looks good to me, if there is no objection, I'll change the >> >> cf status to "Ready for Committer" in few days. >> > >> > Thanks for the review. >> > >> > As I mentioned upthread, I have 2 open points: >> > 1) In the patch I have added a new mat view info parameter to >> > ExplainOneQuery(), do we also need to add it to >> > ExplainOneQuery_hook_type? IMO, we should not (for now), because this >> > would create a backward compatibility issue. >> >> Sorry, I do not know how PostgreSQL handle the backward compatibility issue. >> Is there a guideline? > > I'm not aware of any guidelines as such, but we usually avoid any > changes to existing API, adding/making changes to system catalogs and > so on. > Thanks for explaining, I'd be inclined keep it backward compatibility. >> > 2) Do we document (under respective command pages or somewhere else) >> > that we allow explain/explain analyze for a command? >> > >> >> IMO, we can add a new page to list the commands that can be explain/explain >> analyze, >> since it's clear for users. > > We are listing all the supported commands in explain.sgml, so added > the CREATE MATERIALIZED VIEW(it's missing even though it's supported > prior to this patch) and REFRESH MATERIALIZED VIEW there. > > Attaching v6 patch set. Please have a look. > LGTM. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax
On Sun, 07 Mar 2021 at 19:43, Bharath Rupireddy
wrote:
> I'm reading through the v6 patches again, here are some comments.
>
Thanks for your review again.
> 1) IMO, we can merge 0001 into 0002
> 2) A typo, it's "current" not "ccurrent" - + * Merge ccurrent
> subscription's publications and user specified publications
Fixed.
> 3) In merge_subpublications, do we need to error out or do something
> instead of Assert(!isnull); as in the release build we don't reach
> assert. So, if at all catalogue search returns a null tuple, we don't
> surprise users.
> 4) Can we have a better name for the function merge_subpublications
> say merge_publications (because it's a local function to
> subscriptioncmds.c we don't need "sub" in function name) or any other
> better name?
Rename merge_subpublications to merge_publications as you suggested.
> 5) Instead of doing catalogue look up for the subscriber publications
> in merge_subpublications, why can't we pass in the list from sub =
> GetSubscription(subid, false); (being called in AlterSubscription)
> ---> sub->publications. Do you see any problems in doing so? If done
> that, we can discard the 0001 patch and comments (1) and (3) becomes
> irrelevant.
Thank you point out this. Fixed it in v7 patch set.
Please consider the v7 patch for futher review.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
>From a6ec682b2badb1d6dcb5e41f27bd3d7851353be3 Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Sun, 7 Mar 2021 12:56:55 +
Subject: [PATCH v7 1/4] Introduce a new syntax to add/drop publications
At present, if we want to update publications in subscription, we can
use SET PUBLICATION, however, it requires supply all publications that
exists and the new publications if we want to add new publications, it's
inconvenient. The new syntax only supply the new publications. When
the refresh is true, it only refresh the new publications.
---
src/backend/commands/subscriptioncmds.c | 132
src/backend/parser/gram.y | 20
src/include/nodes/parsenodes.h | 2 +
3 files changed, 154 insertions(+)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index bfd3514546..bf1e579e6f 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -47,6 +47,7 @@
#include "utils/syscache.h"
static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
+static List *merge_publications(List *oldpublist, List *newpublist, bool addpub);
static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err);
@@ -964,6 +965,53 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
break;
}
+ case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
+ case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
+ {
+bool copy_data = false;
+bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
+bool refresh;
+List *publist = NIL;
+
+publist = merge_publications(sub->publications, stmt->publication, isadd);
+
+parse_subscription_options(stmt->options,
+ NULL, /* no "connect" */
+ NULL, NULL, /* no "enabled" */
+ NULL, /* no "create_slot" */
+ NULL, NULL, /* no "slot_name" */
+ isadd ? ©_data : NULL, /* for drop, no "copy_data" */
+ NULL, /* no "synchronous_commit" */
+ &refresh,
+ NULL, NULL, /* no "binary" */
+ NULL, NULL); /* no "streaming" */
+
+values[Anum_pg_subscription_subpublications - 1] =
+ publicationListToArray(publist);
+replaces[Anum_pg_subscription_subpublications - 1] = true;
+
+update_tuple = true;
+
+/* Refresh if user asked us to. */
+if (refresh)
+{
+ if (!sub->enabled)
+ ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions"),
+ errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).")));
+
+ PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with refresh");
+
+ /* Only refresh the added/dropped list of publications. */
+ sub->publications = stmt->publication;
+
+ AlterSubscription_refresh(sub, copy_data);
+}
+
+break;
+ }
+
case ALTER_SUBSCRIPTION_REFRESH:
{
bool copy_data;
@@ -1551,3 +1599,87 @@ ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err)
errhint("Use %s to disassociate the subscription from the slot.",
"ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
}
+
+/*
+ * Merge current
Re: EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW
On Mon, 08 Mar 2021 at 12:28, Bharath Rupireddy wrote: > On Sun, Mar 7, 2021 at 10:13 PM Zhihong Yu wrote: >> Hi, >> >> +* EXPLAIN ANALYZE CREATE TABLE AS or REFRESH MATERIALIZED VIEW >> +* WITH NO DATA is weird. >> >> Maybe it is clearer to spell out WITH NO DATA for both statements, instead >> of sharing it. > > Done that way. > >> - if (!stmt->skipData) >> + if (!stmt->skipData && !explainInfo) >> ... >> + else if (explainInfo) >> >> It would be cleaner to put the 'if (explainInfo)' as the first check. That >> way, the check for skipData can be simplified. > > Changed. > > Thanks for review comments. Attaching v7 patch set with changes only > in 0002 patch. Please have a look. > The v7 patch looks good to me, and there is no other advice, so I change the status to "Ready for Committer". -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Autovacuum and idle_session_timeout
On Thu, 30 Dec 2021 at 17:18, Guillaume Lelarge wrote: > Hello, > > I've been reading the autovacuum code (the launcher and the worker) on the > 14 branch. As previously, I've seen some configuration at the beginning, > especially for statement_timeout, lock_timeout and > idle_in_transaction_session_timeout, and I was surprised to discover there > was no configuration for idle_session_timeout. I'm not sure the code should > set it to 0 as well (otherwise I'd have written a patch), but, if there was > a decision made to ignore its value, I'd be interested to know the reason. > I could guess for the autovacuum worker (it seems to work in a transaction, > so it's already handled by the idle_in_transaction_timeout), but I have no > idea for the autovacuum launcher. > > If it was just missed, I could write a patch this week to fix this. > Oh, it was just missed. I didn't note set autovacuum code set those settings, I think we should also set idle_session_timeout to 0. Should we also change this for pg_dump and pg_backup_archiver? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Autovacuum and idle_session_timeout
On Thu, 30 Dec 2021 at 18:53, Guillaume Lelarge wrote:
> Le jeu. 30 déc. 2021 à 11:44, Japin Li a écrit :
>
>>
> pg_dump works in a single transaction, so it's already dealt with
> idle_in_transaction_timeout. Though I guess setting both would work too.
Attached fix this, please consider reveiew it. Thanks.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index f6d0562876..c7070b05c9 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -602,6 +602,7 @@ AutoVacLauncherMain(int argc, char *argv[])
SetConfigOption("lock_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE);
SetConfigOption("idle_in_transaction_session_timeout", "0",
PGC_SUSET, PGC_S_OVERRIDE);
+ SetConfigOption("idle_session_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE);
/*
* Force default_transaction_isolation to READ COMMITTED. We don't want
@@ -1624,6 +1625,7 @@ AutoVacWorkerMain(int argc, char *argv[])
SetConfigOption("lock_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE);
SetConfigOption("idle_in_transaction_session_timeout", "0",
PGC_SUSET, PGC_S_OVERRIDE);
+ SetConfigOption("idle_session_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE);
/*
* Force default_transaction_isolation to READ COMMITTED. We don't want
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 8903a694ae..15960ea644 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3059,6 +3059,7 @@ _doSetFixedOutputState(ArchiveHandle *AH)
ahprintf(AH, "SET statement_timeout = 0;\n");
ahprintf(AH, "SET lock_timeout = 0;\n");
ahprintf(AH, "SET idle_in_transaction_session_timeout = 0;\n");
+ ahprintf(AH, "SET idle_session_timeout = 0;\n");
/* Select the correct character set encoding */
ahprintf(AH, "SET client_encoding = '%s';\n",
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index b52f3ccda2..fd4f7269c3 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1146,6 +1146,8 @@ setup_connection(Archive *AH, const char *dumpencoding,
ExecuteSqlStatement(AH, "SET lock_timeout = 0");
if (AH->remoteVersion >= 90600)
ExecuteSqlStatement(AH, "SET idle_in_transaction_session_timeout = 0");
+ if (AH->remoteVersion >= 14)
+ ExecuteSqlStatement(AH, "SET idle_session_timeout = 0");
/*
* Quote all identifiers, if requested.
Re: Autovacuum and idle_session_timeout
On Fri, 31 Dec 2021 at 00:24, Tom Lane wrote: > Japin Li writes: >> On Thu, 30 Dec 2021 at 18:53, Guillaume Lelarge >> wrote: >>> pg_dump works in a single transaction, so it's already dealt with >>> idle_in_transaction_timeout. Though I guess setting both would work too. > >> Attached fix this, please consider reveiew it. Thanks. > > This seems rather pointless to me. The idle-session timeout is only > activated in PostgresMain's input loop, so it will never be reached > in autovacuum or other background workers. (The same is true for > idle_in_transaction_session_timeout, so the fact that somebody made > autovacuum.c clear that looks like cargo-cult programming from here, > not useful code.) And as for pg_dump, how would it ever trigger the > timeout? It's not going to sit there thinking, especially not > outside a transaction. > Thanks for your clarify! If the timeout never be reached, should we remove those settings? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Index-only scan for btree_gist turns bpchar to char
On Wed, 05 Jan 2022 at 03:19, Tom Lane wrote:
> Alexander Lakhin writes:
>> While testing the index-only scan fix, I've discovered that replacing
>> the index-only scan with the index scan changes contrib/btree_gist
>> output because index-only scan for btree_gist returns a string without
>> padding.
>
> Ugh, yeah. This seems to be because gbt_bpchar_compress() strips
> trailing spaces (using rtrim1) before storing the value. The
> idea evidently is to simplify gbt_bpchar_consistent, but it's not
> acceptable if the opclass is supposed to support index-only scan.
>
> I see two ways to fix this:
>
> * Disallow index-only scan, by removing the fetch function for this
> opclass. This'd require a module version bump, so people wouldn't
> get that fix automatically.
>
> * Change gbt_bpchar_compress to not trim spaces (it becomes just
> like gbt_text_compress), and adapt gbt_bpchar_consistent to cope.
> This does nothing for the problem immediately, unless you REINDEX
> affected indexes --- but over time an index's entries would get
> replaced with untrimmed versions.
>
> I also wondered if we could make the fetch function reconstruct the
> padding, but it doesn't seem to have access to the necessary info.
>
If we fix this In the second way, the range query has the same results
in both seq scan and index only scan. However, it will incur other
problems. For the following query:
SELECT *, octet_length(a) FROM chartmp WHERE a = '31b0';
Currently, we can get
a | octet_length
--+--
31b0 |4
After fixed, we cannot get any result. For the equal condition,
we must put the extra spaces to make it work.
Here is a patch for POC testing.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/contrib/btree_gist/btree_text.c b/contrib/btree_gist/btree_text.c
index 8019d11281..5fd425047f 100644
--- a/contrib/btree_gist/btree_text.c
+++ b/contrib/btree_gist/btree_text.c
@@ -121,16 +121,7 @@ gbt_bpchar_compress(PG_FUNCTION_ARGS)
}
if (entry->leafkey)
- {
-
- Datum d = DirectFunctionCall1(rtrim1, entry->key);
- GISTENTRY trim;
-
- gistentryinit(trim, d,
- entry->rel, entry->page,
- entry->offset, true);
- retval = gbt_var_compress(&trim, &tinfo);
- }
+ retval = gbt_var_compress(entry, &tinfo);
else
retval = entry;
@@ -179,7 +170,6 @@ gbt_bpchar_consistent(PG_FUNCTION_ARGS)
boolretval;
GBT_VARKEY *key = (GBT_VARKEY *) DatumGetPointer(entry->key);
GBT_VARKEY_R r = gbt_var_key_readable(key);
- void *trim = (void *) DatumGetPointer(DirectFunctionCall1(rtrim1,
PointerGetDatum(query)));
/* All cases served by this function are exact */
*recheck = false;
@@ -189,7 +179,7 @@ gbt_bpchar_consistent(PG_FUNCTION_ARGS)
tinfo.eml = pg_database_encoding_max_length();
}
- retval = gbt_var_consistent(&r, trim, strategy, PG_GET_COLLATION(),
+ retval = gbt_var_consistent(&r, query, strategy, PG_GET_COLLATION(),
GIST_LEAF(entry), &tinfo, fcinfo->flinfo);
PG_RETURN_BOOL(retval);
}
Re: Index-only scan for btree_gist turns bpchar to char
On Thu, 06 Jan 2022 at 00:34, Tom Lane wrote:
> Japin Li writes:
>> Here is a patch for POC testing.
>
> This is certainly not right. You've made gbt_bpchar_consistent
> work identically to gbt_text_consistent, but it needs to implement
> a test equivalent to bpchareq, ie ignore trailing spaces in both
> inputs.
>
Thanks for your explaintion! The bpchareq already ignore trailing spaces
in both inputs. The question is that the bpchar in btree_gist do not call
bpchareq, it always call texteq. I tried the patch[1] and it works as
expected, howerver, I don't think it's good way to fix this problem.
> The minimum-effort fix would be to apply rtrim1 to both strings
> in gbt_bpchar_consistent, but I wonder if we can improve on that
> by pushing the ignore-trailing-spaces behavior further down.
> I didn't look yet at whether gbt_var_consistent can support
> any type-specific behavior.
>
Adding type-specific for gbt_var_consistent looks like more generally.
For example, for bpchar type, we should call bpchareq rather than texteq.
Am I understand right?
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
[1]
diff --git a/contrib/btree_gist/btree_text.c b/contrib/btree_gist/btree_text.c
index 8019d11281..7f45ee6e3b 100644
--- a/contrib/btree_gist/btree_text.c
+++ b/contrib/btree_gist/btree_text.c
@@ -121,16 +121,7 @@ gbt_bpchar_compress(PG_FUNCTION_ARGS)
}
if (entry->leafkey)
- {
-
- Datum d = DirectFunctionCall1(rtrim1, entry->key);
- GISTENTRY trim;
-
- gistentryinit(trim, d,
- entry->rel, entry->page,
- entry->offset, true);
- retval = gbt_var_compress(&trim, &tinfo);
- }
+ retval = gbt_var_compress(entry, &tinfo);
else
retval = entry;
@@ -189,6 +180,11 @@ gbt_bpchar_consistent(PG_FUNCTION_ARGS)
tinfo.eml = pg_database_encoding_max_length();
}
+ r.lower = (bytea *) DatumGetPointer(DirectFunctionCall1(rtrim1,
+
PointerGetDatum(r.lower)));
+ r.upper = (bytea *) DatumGetPointer(DirectFunctionCall1(rtrim1,
+
PointerGetDatum(r.upper)));
+
retval = gbt_var_consistent(&r, trim, strategy, PG_GET_COLLATION(),
GIST_LEAF(entry), &tinfo, fcinfo->flinfo);
PG_RETURN_BOOL(retval);
Re: Support tab completion for upper character inputs in psql
On Fri, 07 Jan 2022 at 10:12, tanghy.f...@fujitsu.com
wrote:
> On Thursday, January 6, 2022 11:57 PM, Tom Lane wrote:
>>
>> Peter Eisentraut writes:
>> > So the ordering of the suggested completions is different. I don't know
>> > offhand how that ordering is determined. Perhaps it's dependent on
>> > locale, readline version, or operating system. In any case, we need to
>> > figure this out to make this test stable.
>>
>> I don't think we want to get into the business of trying to make that
>> consistent across different readline/libedit versions. How about
>> adjusting the test case so that only one enum value is to be printed?
>>
>
> Thanks for your suggestion. Agreed.
> Fixed the test case to show only one enum value.
>
+/*
+ * pg_string_tolower - Fold a string to lower case if the string is not quoted
+ * and only contains ASCII characters.
+ * For German/Turkish etc text, no change will be made.
+ *
+ * The returned value has to be freed.
+ */
+static char *
+pg_string_tolower_if_ascii(const char *text)
+{
s/pg_string_tolower/pg_string_tolower_if_ascii/ for comments.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: Index-only scan for btree_gist turns bpchar to char
On Fri, 07 Jan 2022 at 03:21, Tom Lane wrote: > I looked at this and it does seem like it might work, as per attached > patch. The one thing that is troubling me is that the opclass is set > up to apply gbt_text_same, which is formally the Wrong Thing for bpchar, > because the equality semantics shouldn't be quite the same. But we > could not fix that without a module version bump, which is annoying. > I think that it might not be necessary to change it, because > > (1) There's no such thing as unique GIST indexes, so it should not > matter if the "same" function is a bit stricter than the datatype's > nominal notion of equality. It's certainly okay for that to vary > from the semantics applied by the consistent function --- GIST has > no idea that the consistent function is allegedly testing equality. > > (2) If all the input values for a column have been coerced to the same > typmod, then it doesn't matter because two values that are equal after > space-stripping would be equal without space-stripping, too. > > However, (2) doesn't hold for an existing index that the user has failed > to REINDEX, because then the index would contain some space-stripped > values that same() will not say are equal to incoming new values. > Again, I think this doesn't matter much, but maybe I'm missing > something. I've not really dug into what GIST uses the same() > function for. > > In any case, if we do need same() to implement the identical > behavior to bpchareq(), then the other solution isn't sufficient > either. > > So in short, it seems like we ought to do some compatibility testing > and see if this code misbehaves at all with an index created by the > old code. I don't particularly want to do that ... any volunteers? > Thanks for your patch, it looks good to me. I'm not sure how to test this. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW
On Tue, 16 Mar 2021 at 20:13, Bharath Rupireddy wrote: > On Tue, Mar 16, 2021 at 1:15 AM Tom Lane wrote: >> >> [ Sorry for not looking at this thread sooner ] >> >> Bharath Rupireddy writes: >> > Currently, $subject is not allowed. We do plan the mat view query >> > before every refresh. I propose to show the explain/explain analyze of >> > the select part of the mat view in case of Refresh Mat View(RMV). >> >> TBH, I think we should reject this. The problem with it is that it >> binds us to the assumption that REFRESH MATERIALIZED VIEW has an >> explainable plan. There are various people poking at ideas like >> incremental matview updates, which might rely on some implementation >> that doesn't exactly equate to a SQL query. Incremental updates are >> hard enough already; they'll be even harder if they also have to >> maintain compatibility with a pre-existing EXPLAIN behavior. >> >> I don't really see that this feature buys us anything you can't >> get by explaining the view's query, so I think we're better advised >> to keep our options open about how REFRESH might be implemented >> in future. > > That makes sense to me. Thanks for the comments. I'm fine to withdraw the > patch. > > I would like to see if the 0001 patch(attaching here) will be useful > at all. It just splits up the existing ExecRefreshMatView into a few > functions to make the code readable. +1. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax
On Mon, 22 Mar 2021 at 11:14, Bharath Rupireddy
wrote:
> On Sun, Mar 7, 2021 at 7:21 PM Japin Li wrote:
>> Thank you point out this. Fixed it in v7 patch set.
>>
>> Please consider the v7 patch for futher review.
>
> Thanks for the patches. I just found the following behaviour with the
> new ADD/DROP syntax: when the specified publication list has
> duplicates, the patch is throwing "publication is already present"
> error. It's adding the first instance of the duplicate into the list
> and the second instance is being checked in the added list and
> throwing the "already present error". The error message means that the
> publication is already present in the subscription but it's not true.
> See my testing at [1].
>
> I think we have two cases:
> case 1: the publication/s specified in the new ADD/DROP syntax may/may
> not have already been associated with the subscription, so the error
> "publication is already present"/"publication doesn't exist" error
> makes sense.
> case 2: there can be duplicate publications specified in the new
> ADD/DROP syntax, in this case the error "publication name "mypub2"
> used more than once" makes more sense much like [2].
>
> [1]
> postgres=# select subpublications from pg_subscription;
> subpublications
> -
> {mypub,mypub1}
>
> postgres=# alter subscription mysub add publication mypub2, mypub2;
> ERROR: publication "mypub2" is already present in the subscription
>
> postgres=# select subpublications from pg_subscription;
> subpublications
> ---
> {mypub,mypub1,mypub2}
>
> postgres=# alter subscription mysub drop publication mypub2, mypub2;
> ERROR: publication "mypub2" doesn't exist in the subscription
>
> [2]
> postgres=# alter subscription mysub set publication mypub2, mypub2;
> ERROR: publication name "mypub2" used more than once
>
Thanks for your review.
I check the duplicates for newpublist in merge_publications(). The code is
copied from publicationListToArray().
I do not check for all duplicates because it will make the code more complex.
For example:
ALTER SUBSCRIPTION mysub ADD PUBLICATION mypub2, mypub2, mypub2;
If we record the duplicate publication names in list A, when we find a
duplication in newpublist, we should check whether the publication is
in list A or not, to make the error message make sense (do not have
duplicate publication names in error message).
Thought?
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
>From 632505be60af48a8d9514e58d536f3acaff48550 Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Sun, 7 Mar 2021 12:56:55 +
Subject: [PATCH v8 1/4] Introduce a new syntax to add/drop publications
At present, if we want to update publications in subscription, we can
use SET PUBLICATION, however, it requires supply all publications that
exists and the new publications if we want to add new publications, it's
inconvenient. The new syntax only supply the new publications. When
the refresh is true, it only refresh the new publications.
---
src/backend/commands/subscriptioncmds.c | 153
src/backend/parser/gram.y | 20
src/include/nodes/parsenodes.h | 2 +
3 files changed, 175 insertions(+)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index bfd3514546..368ee36961 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -47,6 +47,7 @@
#include "utils/syscache.h"
static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
+static List *merge_publications(List *oldpublist, List *newpublist, bool addpub);
static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err);
@@ -964,6 +965,53 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
break;
}
+ case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
+ case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
+ {
+bool copy_data = false;
+bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
+bool refresh;
+List *publist = NIL;
+
+publist = merge_publications(sub->publications, stmt->publication, isadd);
+
+parse_subscription_options(stmt->options,
+ NULL, /* no "connect" */
+ NULL, NULL, /* no "enabled" */
+ NULL, /* no "create_slot" */
+ NULL, NULL, /* no "slot_name" */
+ isadd ? ©_data : NULL, /* for drop, no "copy_data" */
+ NULL, /* no "synchronous_commit" */
+ &refresh,
+ NULL, NULL, /* no "binary" */
+ NULL, NULL); /* no &
Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax
On Sat, 03 Apr 2021 at 13:20, Bharath Rupireddy wrote: > On Sat, Apr 3, 2021 at 1:29 AM Peter Eisentraut > wrote: >> The code you have in merge_publications() to report all existing >> publications is pretty messy and is not properly internationalized. I >> think what you are trying to do there is excessive. Compare this >> similar case: >> >> create table t1 (a int, b int); >> alter table t1 add column a int, add column b int; >> ERROR: 42701: column "a" of relation "t1" already exists >> >> I think you can make both this and the duplicate checking much simpler >> if you just report the first conflict. > > Yes, we are erroring out on the first conflict for both duplicates and > in merge_publications. > >> I think this patch is about ready to commit, but please provide a final >> version in good time. > > I took the liberty to address all the review comments and provide a v9 > patch on top of Japin's v8 patch-set. > >> (Also, please combine your patches into a single patch.) > > Done. > > Attaching v9 patch, please review it. > Sorry for the late reply! Thanks for your updating the new patch, and it looks good to me. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax
On Tue, 06 Apr 2021 at 17:56, Peter Eisentraut wrote: > On 06.04.21 07:24, Japin Li wrote: >>>> I think this patch is about ready to commit, but please provide a final >>>> version in good time. >>> I took the liberty to address all the review comments and provide a v9 >>> patch on top of Japin's v8 patch-set. >>> >>>> (Also, please combine your patches into a single patch.) >>> Done. >>> >>> Attaching v9 patch, please review it. >>> >> Sorry for the late reply! Thanks for your updating the new patch, and it >> looks >> good to me. > > Committed. > > Note that you can use syntax like "ADD|DROP|SET" in the tab completion > code. I have simplified some of your code like that. > > I also deduplicated the documentation additions a bit. Thanks! -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Truncate in synchronous logical replication failed
On Wed, 07 Apr 2021 at 16:34, tanghy.f...@fujitsu.com
wrote:
> On Wednesday, April 7, 2021 5:28 PM Amit Kapila
> wrote
>
>>Can you please check if the behavior is the same for PG-13? This is
>>just to ensure that we have not introduced any bug in PG-14.
>
> Yes, same failure happens at PG-13, too.
>
I found that when we truncate a table in synchronous logical replication,
LockAcquireExtended() [1] will try to take a lock via fast path and it
failed (FastPathStrongRelationLocks->count[fasthashcode] = 1).
However, it can acquire the lock when in asynchronous logical replication.
I'm not familiar with the locks, any suggestions? What the difference
between sync and async logical replication for locks?
[1]
if (EligibleForRelationFastPath(locktag, lockmode) &&
FastPathLocalUseCount < FP_LOCK_SLOTS_PER_BACKEND)
{
uint32 fasthashcode = FastPathStrongLockHashPartition(hashcode);
boolacquired;
/*
* LWLockAcquire acts as a memory sequencing point, so it's safe to
* assume that any strong locker whose increment to
* FastPathStrongRelationLocks->counts becomes visible after we test
* it has yet to begin to transfer fast-path locks.
*/
LWLockAcquire(&MyProc->fpInfoLock, LW_EXCLUSIVE);
if (FastPathStrongRelationLocks->count[fasthashcode] != 0)
acquired = false;
else
acquired = FastPathGrantRelationLock(locktag->locktag_field2,
lockmode);
LWLockRelease(&MyProc->fpInfoLock);
if (acquired)
{
/*
* The locallock might contain stale pointers to some old shared
* objects; we MUST reset these to null before considering the
* lock to be acquired via fast-path.
*/
locallock->lock = NULL;
locallock->proclock = NULL;
GrantLockLocal(locallock, owner);
return LOCKACQUIRE_OK;
}
}
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: Truncate in synchronous logical replication failed
On Thu, 08 Apr 2021 at 19:20, Japin Li wrote: > On Wed, 07 Apr 2021 at 16:34, tanghy.f...@fujitsu.com > wrote: >> On Wednesday, April 7, 2021 5:28 PM Amit Kapila >> wrote >> >>>Can you please check if the behavior is the same for PG-13? This is >>>just to ensure that we have not introduced any bug in PG-14. >> >> Yes, same failure happens at PG-13, too. >> > > I found that when we truncate a table in synchronous logical replication, > LockAcquireExtended() [1] will try to take a lock via fast path and it > failed (FastPathStrongRelationLocks->count[fasthashcode] = 1). > However, it can acquire the lock when in asynchronous logical replication. > I'm not familiar with the locks, any suggestions? What the difference > between sync and async logical replication for locks? > After some analyze, I find that when the TRUNCATE finish, it will call SyncRepWaitForLSN(), for asynchronous logical replication, it will exit early, and then it calls ResourceOwnerRelease(RESOURCE_RELEASE_LOCKS) to release the locks, so the walsender can acquire the lock. But for synchronous logical replication, SyncRepWaitForLSN() will wait for specified LSN to be confirmed, so it cannot release the lock, and the walsender try to acquire the lock. Obviously, it cannot acquire the lock, because the lock hold by the process which performs TRUNCATE command. This is why the TRUNCATE in synchronous logical replication is blocked. I don't know if it makes sense to fix this, if so, how to do fix it? Thoughts? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, 14 Oct 2021 at 19:49, Dilip Kumar wrote:
> On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro wrote:
>>
>> Hi All,
>>
>> Publisher 'DateStyle' is set as "SQL, MDY", whereas in Subscriber as
>> "SQL, DMY", the logical replication is not working...
>>
>> From Publisher:
>> postgres=# INSERT INTO calendar VALUES ('07-18-1036', '1'), ('05-15-1135',
>> '1');
>> INSERT 0 2
>>
>> Getting below error in the subscriber log file,
>> 2021-10-14 00:59:23.067 PDT [38262] ERROR: date/time field value out
>> of range: "07/18/1036"
>> 2021-10-14 00:59:23.067 PDT [38262] HINT: Perhaps you need a
>> different "datestyle" setting.
>>
>> Is this an expected behavior?
>
> Looks like a problem to me, I think for fixing this, on logical
> replication connection always set subscriber's DateStlyle, with that
> the walsender will always send the data in the same DateStyle that
> worker understands and then we are good.
Right! Attached fix it.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 5c6e56a5b2..81cf9e30d7 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -30,6 +30,7 @@
#include "pqexpbuffer.h"
#include "replication/walreceiver.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/pg_lsn.h"
#include "utils/tuplestore.h"
@@ -218,6 +219,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
if (logical)
{
PGresult *res;
+ const char *datestyle;
res = libpqrcv_PQexec(conn->streamConn,
ALWAYS_SECURE_SEARCH_PATH_SQL);
@@ -229,6 +231,23 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
pchomp(PQerrorMessage(conn->streamConn);
}
PQclear(res);
+
+ datestyle = GetConfigOption("datestyle", true, true);
+ if (datestyle != NULL) {
+ char *sql;
+
+ sql = psprintf("SELECT pg_catalog.set_config('datestyle', '%s', false);", datestyle);
+ res = libpqrcv_PQexec(conn->streamConn, sql);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+PQclear(res);
+ereport(ERROR,
+ (errmsg("could not set datestyle: %s",
+pchomp(PQerrorMessage(conn->streamConn);
+ }
+ PQclear(res);
+ pfree(sql);
+ }
}
conn->logical = logical;
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Sat, 16 Oct 2021 at 22:42, Japin Li wrote:
> On Thu, 14 Oct 2021 at 19:49, Dilip Kumar wrote:
>> On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro wrote:
>>>
>>> Hi All,
>>>
>>> Publisher 'DateStyle' is set as "SQL, MDY", whereas in Subscriber as
>>> "SQL, DMY", the logical replication is not working...
>>>
>>> From Publisher:
>>> postgres=# INSERT INTO calendar VALUES ('07-18-1036', '1'), ('05-15-1135',
>>> '1');
>>> INSERT 0 2
>>>
>>> Getting below error in the subscriber log file,
>>> 2021-10-14 00:59:23.067 PDT [38262] ERROR: date/time field value out
>>> of range: "07/18/1036"
>>> 2021-10-14 00:59:23.067 PDT [38262] HINT: Perhaps you need a
>>> different "datestyle" setting.
>>>
>>> Is this an expected behavior?
>>
>> Looks like a problem to me, I think for fixing this, on logical
>> replication connection always set subscriber's DateStlyle, with that
>> the walsender will always send the data in the same DateStyle that
>> worker understands and then we are good.
>
> Right! Attached fix it.
Add a test case in subscription/t/100_bugs.pl. Please consider the v2 patch
for review.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 5c6e56a5b2..81cf9e30d7 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -30,6 +30,7 @@
#include "pqexpbuffer.h"
#include "replication/walreceiver.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/pg_lsn.h"
#include "utils/tuplestore.h"
@@ -218,6 +219,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
if (logical)
{
PGresult *res;
+ const char *datestyle;
res = libpqrcv_PQexec(conn->streamConn,
ALWAYS_SECURE_SEARCH_PATH_SQL);
@@ -229,6 +231,23 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
pchomp(PQerrorMessage(conn->streamConn);
}
PQclear(res);
+
+ datestyle = GetConfigOption("datestyle", true, true);
+ if (datestyle != NULL) {
+ char *sql;
+
+ sql = psprintf("SELECT pg_catalog.set_config('datestyle', '%s', false);", datestyle);
+ res = libpqrcv_PQexec(conn->streamConn, sql);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+PQclear(res);
+ereport(ERROR,
+ (errmsg("could not set datestyle: %s",
+pchomp(PQerrorMessage(conn->streamConn);
+ }
+ PQclear(res);
+ pfree(sql);
+ }
}
conn->logical = logical;
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index baa4a90771..a88a61df41 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 5;
+use Test::More tests => 6;
# Bug #15114
@@ -224,3 +224,44 @@ $node_sub->safe_psql('postgres', "DROP TABLE tab1");
$node_pub->stop('fast');
$node_pub_sub->stop('fast');
$node_sub->stop('fast');
+
+# Verify different datestyle between publisher and subscriber.
+$node_publisher = PostgresNode->new('datestyle_publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+ "datestyle = 'SQL, MDY'");
+$node_publisher->start;
+
+$node_subscriber = PostgresNode->new('datestyle_subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->append_conf('postgresql.conf',
+ "datestyle = 'SQL, DMY'");
+$node_subscriber->start;
+
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_rep VALUES ('07-18-2021'), ('05-15-2018')");
+
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+
+# Setup logical replication
+my $node_publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tab_pub FOR ALL TABLES");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tab_sub CONNECTION '$node_publisher_connstr' PUBLICATION tab_pub");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM tab_rep");
+is($result, qq(2), 'failed to replication date from different datestyle');
+
+# Clean up the tables on both publisher and subscriber as we don't need them
+$node_publisher->safe_psql('postgres', 'DROP TABLE tab_rep');
+$node_subscriber->safe_psql('postgres', 'DROP TABLE tab_rep');
+
+$node_publisher->stop('fast');
+$node_subscriber->stop('fast');
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 11:14, Sadhuprasad Patro wrote: >> Add a test case in subscription/t/100_bugs.pl. Please consider the v2 patch >> for review. >> > > Reviewed and tested the patch, it works fine... There are some > trailing spaces present in the newly added code lines, which needs to > be corrected... > Doing some further testing with different datestyles, will update further... > Thanks for your review and test! As Tom Lane said, the postgres_fdw has the similar things, I will update the patch later. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 11:59, Michael Paquier wrote: > On Sun, Oct 17, 2021 at 11:41:35PM -0400, Tom Lane wrote: >> Ah ... see postgres_fdw's set_transmission_modes(). I think we want >> to copy that logic not invent some other way to do it. > > dblink.c has something similar as of applyRemoteGucs(), except that it > does not do extra_float_digits. It would be nice to avoid more > duplication for those things, at least on HEAD. On the top of my > head, don't we have something similar for parallel workers when > passing down GUCs from the leader? Since it will be used in more than one places. IMO, we can implement it in core. Any thoughts? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 11:41, Tom Lane wrote: > I wrote: >> An alternative that wouldn't require a network round trip is for the >> publisher to set its own datestyle to ISO/YMD. I'm pretty sure that >> will be interpreted correctly regardless of the receiver's datestyle. > > Ah ... see postgres_fdw's set_transmission_modes(). I think we want > to copy that logic not invent some other way to do it. > Thanks for your remention. As Michael Paquier side, dblink also uses the similar logical. I will read them then update the patch. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 12:17, Tom Lane wrote:
> Japin Li writes:
>> On Mon, 18 Oct 2021 at 11:59, Michael Paquier wrote:
>>> dblink.c has something similar as of applyRemoteGucs(), except that it
>>> does not do extra_float_digits. It would be nice to avoid more
>>> duplication for those things, at least on HEAD. On the top of my
>>> head, don't we have something similar for parallel workers when
>>> passing down GUCs from the leader?
>
>> Since it will be used in more than one places. IMO, we can implement it in
>> core.
>> Any thoughts?
>
> It's not going to be the same code everywhere. A logrep sender won't
> have a need to save-and-restore the settings like postgres_fdw does,
Thanks for your explanation. Yeah, we do not need reset the settings in
logical replication.
> AFAICS. Also, now that I look at it, dblink is doing the opposite
> thing of absorbing the sender's values.
>
Sorry I misunderstand. You are right, the dblink applies the remote
server's settings to local server.
Attached v3 patch modify the settings on sender as you suggest.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e2a76ba055..3c1b2fa85e 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2223,6 +2223,20 @@ retry1:
{
am_walsender = true;
am_db_walsender = true;
+
+ /*
+ * Force assorted GUC parameters to settings that ensure
+ * that we'll output data values in a form that is
+ * unambiguous to the walreceiver.
+ */
+ port->guc_options = lappend(port->guc_options,
+pstrdup("datestyle"));
+ port->guc_options = lappend(port->guc_options,
+pstrdup("ISO"));
+ port->guc_options = lappend(port->guc_options,
+pstrdup("intervalstyle"));
+ port->guc_options = lappend(port->guc_options,
+pstrdup("postgres"));
}
else if (!parse_bool(valptr, &am_walsender))
ereport(FATAL,
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index baa4a90771..a88a61df41 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 5;
+use Test::More tests => 6;
# Bug #15114
@@ -224,3 +224,44 @@ $node_sub->safe_psql('postgres', "DROP TABLE tab1");
$node_pub->stop('fast');
$node_pub_sub->stop('fast');
$node_sub->stop('fast');
+
+# Verify different datestyle between publisher and subscriber.
+$node_publisher = PostgresNode->new('datestyle_publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+ "datestyle = 'SQL, MDY'");
+$node_publisher->start;
+
+$node_subscriber = PostgresNode->new('datestyle_subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->append_conf('postgresql.conf',
+ "datestyle = 'SQL, DMY'");
+$node_subscriber->start;
+
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_rep VALUES ('07-18-2021'), ('05-15-2018')");
+
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+
+# Setup logical replication
+my $node_publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tab_pub FOR ALL TABLES");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tab_sub CONNECTION '$node_publisher_connstr' PUBLICATION tab_pub");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM tab_rep");
+is($result, qq(2), 'failed to replication date from different datestyle');
+
+# Clean up the tables on both publisher and subscriber as we don't need them
+$node_publisher->safe_psql('postgres', 'DROP TABLE tab_rep');
+$node_subscriber->safe_psql('postgres', 'DROP TABLE tab_rep');
+
+$node_publisher->stop('fast');
+$node_subscriber->stop('fast');
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 11:26, Masahiko Sawada wrote:
> On Thu, Oct 14, 2021 at 8:50 PM Dilip Kumar wrote:
>>
>> On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro wrote:
>> >
>> > Hi All,
>> >
>> > Publisher 'DateStyle' is set as "SQL, MDY", whereas in Subscriber as
>> > "SQL, DMY", the logical replication is not working...
>> >
>> > From Publisher:
>> > postgres=# INSERT INTO calendar VALUES ('07-18-1036', '1'), ('05-15-1135',
>> > '1');
>> > INSERT 0 2
>> >
>> > Getting below error in the subscriber log file,
>> > 2021-10-14 00:59:23.067 PDT [38262] ERROR: date/time field value out
>> > of range: "07/18/1036"
>> > 2021-10-14 00:59:23.067 PDT [38262] HINT: Perhaps you need a
>> > different "datestyle" setting.
>> >
>> > Is this an expected behavior?
>>
>> Looks like a problem to me, I think for fixing this, on logical
>> replication connection always set subscriber's DateStlyle, with that
>> the walsender will always send the data in the same DateStyle that
>> worker understands and then we are good.
>
> +1
>
> Probably the same is true for IntervalStyle? If the publisher sets
> 'sql_standard', the subscriber sets 'postgres', and an interval value
> '-1 11:22:33' is inserted, these two nodes have different data:
>
> * Publisher
> =# set intervalstyle to 'postgres'; select * from test;
> i
> ---
> -1 days -11:22:33
> (1 row)
>
> * Subscriber
> =# set intervalstyle to 'postgres'; select * from test;
> i
> ---
> -1 days +11:22:33
> (1 row)
>
I attached v3 patch that set IntervalStyle to 'postgres' when the
server backend is walsender, and this problem has gone.
I test that set IntervalStyle to 'sql_standard' on publisher and
'iso_8601' on subscriber, it works fine.
Please try v3 patch and let me know if they work as unexpected.
Thanks in advance.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: UPDATE on Domain Array that is based on a composite key crashes
On Tue, 19 Oct 2021 at 17:12, Zhihong Yu wrote:
> On Tue, Oct 19, 2021 at 12:39 AM Onder Kalaci wrote:
>
>> Hi hackers,
>>
>>
>>
>> I couldn’t find a similar report to this one, so starting a new thread. I
>> can reproduce this on v14.0 as well as PostgreSQL 12.5 (not tried below
>> versions).
>>
>>
>>
>> Steps to reproduce:
>>
>>
>>
>> CREATE TYPE two_ints as (if1 int, if2 int);
>>
>> CREATE DOMAIN domain AS two_ints CHECK ((VALUE).if1 > 0);
>>
>> CREATE TABLE domain_indirection_test (f1 int, f3 domain, domain_array
>> domain[]);
>>
>> INSERT INTO domain_indirection_test (f1,f3.if1) VALUES (0, 1);
>>
>> UPDATE domain_indirection_test SET domain_array[0].if2 = 5;
>>
>> server closed the connection unexpectedly
>>
>> This probably means the server terminated abnormally
>>
>> before or while processing the request.
>>
>> The connection to the server was lost. Attempting reset: Failed.
>>
>> Time: 3.990 ms
>>
>
> Hi,
> It seems the following change would fix the crash:
>
> diff --git a/src/postgres/src/backend/executor/execExprInterp.c
> b/src/postgres/src/backend/executor/execExprInterp.c
> index 622cab9d4..50cb4f014 100644
> --- a/src/postgres/src/backend/executor/execExprInterp.c
> +++ b/src/postgres/src/backend/executor/execExprInterp.c
> @@ -3038,6 +3038,9 @@ ExecEvalFieldStoreDeForm(ExprState *state,
> ExprEvalStep *op, ExprContext *econte
> HeapTupleHeader tuphdr;
> HeapTupleData tmptup;
>
> +if (DatumGetPointer(tupDatum) == NULL) {
> +return;
> +}
> tuphdr = DatumGetHeapTupleHeader(tupDatum);
> tmptup.t_len = HeapTupleHeaderGetDatumLength(tuphdr);
> ItemPointerSetInvalid(&(tmptup.t_self));
>
> Here is the result after the update statement:
>
> # UPDATE domain_indirection_test SET domain_array[0].if2 = 5;
> UPDATE 1
> # select * from domain_indirection_test;
> f1 | f3 | domain_array
> +--+
> 0 | (1,) | [0:0]={"(,5)"}
> (1 row)
>
Yeah, it fixes the core dump.
However, When I test the patch, I find the update will replace all data
in `domain` if we only update one field.
postgres=# UPDATE domain_indirection_test SET domain_array[0].if2 = 5;
UPDATE 1
postgres=# select * from domain_indirection_test ;
f1 | f3 | domain_array
+--+
0 | (1,) | [0:0]={"(,5)"}
(1 row)
postgres=# UPDATE domain_indirection_test SET domain_array[0].if1 = 10;
UPDATE 1
postgres=# select * from domain_indirection_test ;
f1 | f3 | domain_array
+--+-
0 | (1,) | [0:0]={"(10,)"}
(1 row)
So I try to update all field in `domain`, and find only the last one will
be stored.
postgres=# UPDATE domain_indirection_test SET domain_array[0].if1 = 10,
domain_array[0].if2 = 5;
UPDATE 1
postgres=# select * from domain_indirection_test ;
f1 | f3 | domain_array
+--+
0 | (1,) | [0:0]={"(,5)"}
(1 row)
postgres=# UPDATE domain_indirection_test SET domain_array[0].if2 = 10,
domain_array[0].if1 = 5;
UPDATE 1
postgres=# select * from domain_indirection_test ;
f1 | f3 | domain_array
+--+
0 | (1,) | [0:0]={"(5,)"}
(1 row)
Does this worked as expected? For me, For me, I think this is a bug.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Unexpected behavior of updating domain array that is based on a composite
Hi, hackers
While reading the patch in [1], I found there is an unexpected behavior when
update the domain array that is based on a composite.
Steps to reproduce:
1)
CREATE TYPE two_ints as (if1 int, if2 int);
CREATE DOMAIN domain AS two_ints CHECK ((VALUE).if1 > 0);
CREATE TABLE domain_indirection_test (f1 int, f3 domain, domain_array domain[]);
INSERT INTO domain_indirection_test (f1,f3.if1) VALUES (0, 1);
2) The following test besed on patch in [1].
UPDATE domain_indirection_test SET domain_array[0].if2 = 5;
select * from domain_indirection_test;
f1 | f3 | domain_array
+--+
0 | (1,) | [0:0]={"(,5)"}
3)
UPDATE domain_indirection_test SET domain_array[0].if1 = 10;
select * from domain_indirection_test ;
f1 | f3 | domain_array
+--+-
0 | (1,) | [0:0]={"(10,)"}
(1 row)
4)
UPDATE domain_indirection_test SET domain_array[0].if1 = 10,
domain_array[0].if2 = 5;
select * from domain_indirection_test ;
f1 | f3 | domain_array
+--+
0 | (1,) | [0:0]={"(,5)"}
(1 row)
5)
UPDATE domain_indirection_test SET domain_array[0].if2 = 10,
domain_array[0].if1 = 5;
select * from domain_indirection_test ;
f1 | f3 | domain_array
+--+
0 | (1,) | [0:0]={"(5,)"}
(1 row)
6) Work as expected.
UPDATE domain_indirection_test SET domain_array[0] = (10, 5);
select * from domain_indirection_test ;
f1 | f3 | domain_array
+--+--
0 | (1,) | [0:0]={"(10,5)"}
(1 row)
[1]
https://www.postgresql.org/message-id/PH0PR21MB132823A46AA36F0685B7A29AD8BD9%40PH0PR21MB1328.namprd21.prod.outlook.com
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: UPDATE on Domain Array that is based on a composite key crashes
, queryEnv=0x, dest=,
>>> completionTag="") at pquery.c:161:2
>>>
>>> frame #17: 0x000103577c5e
>>> postgres`PortalRunMulti(portal=0x7f9215024110, isTopLevel=true,
>>> setHoldSnapshot=false, dest=0x7f9211818c38, altdest=0x7f9211818c38,
>>> completionTag="") at pquery.c:0
>>>
>>> frame #18: 0x00010357763d
>>> postgres`PortalRun(portal=0x7f9215024110, count=9223372036854775807,
>>> isTopLevel=, run_once=, dest=0x7f9211818c38,
>>> altdest=0x7f9211818c38, completionTag="") at pquery.c:796:5
>>>
>>> frame #19: 0x000103574f87
>>> postgres`exec_simple_query(query_string="UPDATE domain_indirection_test SET
>>> domain_array[0].if2 = 5;") at postgres.c:1215:10
>>>
>>> frame #20: 0x0001035746b8
>>> postgres`PostgresMain(argc=, argv=,
>>> dbname=, username=) at postgres.c:0
>>>
>>> frame #21: 0x00010350d712 postgres`BackendRun(port=)
>>> at postmaster.c:4494:2
>>>
>>> frame #22: 0x00010350cffa
>>> postgres`BackendStartup(port=) at postmaster.c:4177:3
>>>
>>> frame #23: 0x00010350c59c postgres`ServerLoop at
>>> postmaster.c:1725:7
>>>
>>> frame #24: 0x00010350ac8d postgres`PostmasterMain(argc=3,
>>> argv=0x7f9210d049c0) at postmaster.c:1398:11
>>>
>>> frame #25: 0x00010347fbdd postgres`main(argc=,
>>> argv=) at main.c:228:3
>>>
>>> frame #26: 0x7fff204e8f3d libdyld.dylib`start + 1
>>>
>>>
>>>
>>>
>>>
>>> Thanks,
>>>
>>> Onder KALACI
>>>
>>> Software Engineer at Microsoft &
>>>
>>> Developing the Citus database extension for PostgreSQL
>>>
>>
>> Hi,
>> It seems the following change would fix the crash:
>>
>> diff --git a/src/postgres/src/backend/executor/execExprInterp.c
>> b/src/postgres/src/backend/executor/execExprInterp.c
>> index 622cab9d4..50cb4f014 100644
>> --- a/src/postgres/src/backend/executor/execExprInterp.c
>> +++ b/src/postgres/src/backend/executor/execExprInterp.c
>> @@ -3038,6 +3038,9 @@ ExecEvalFieldStoreDeForm(ExprState *state,
>> ExprEvalStep *op, ExprContext *econte
>> HeapTupleHeader tuphdr;
>> HeapTupleData tmptup;
>>
>> +if (DatumGetPointer(tupDatum) == NULL) {
>> +return;
>> +}
>> tuphdr = DatumGetHeapTupleHeader(tupDatum);
>> tmptup.t_len = HeapTupleHeaderGetDatumLength(tuphdr);
>> ItemPointerSetInvalid(&(tmptup.t_self));
>>
>> Here is the result after the update statement:
>>
>> # UPDATE domain_indirection_test SET domain_array[0].if2 = 5;
>> UPDATE 1
>> # select * from domain_indirection_test;
>> f1 | f3 | domain_array
>> +--+
>> 0 | (1,) | [0:0]={"(,5)"}
>> (1 row)
>>
>> Cheers
>>
> Hi,
> Here is the patch.
Thanks for your updated the patch. A minor code style, we can remove the
braces when there is only one statement, this is more consenting with the
codebase. Others looks good to me.
> If the new test should be placed in a different .sql file, please let me
> know.
>
> The update issue Japin mentioned seems to be orthogonal to the crash.
>
I start a new thread to discuss it [1].
[1]
https://www.postgresql.org/message-id/meyp282mb1669bed5cefe711e00c7421eb6...@meyp282mb1669.ausp282.prod.outlook.com
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 17:27, Dilip Kumar wrote:
> On Mon, Oct 18, 2021 at 1:41 PM Japin Li wrote:
>
>> I attached v3 patch that set IntervalStyle to 'postgres' when the
>> server backend is walsender, and this problem has gone.
>
>> I test that set IntervalStyle to 'sql_standard' on publisher and
>> 'iso_8601' on subscriber, it works fine.
>
>> Please try v3 patch and let me know if they work as unexpected.
>> Thanks in advance.
>
> I think the idea of setting the standard DateStyle and the
> IntervalStyle on the walsender process looks fine to me. As this will
> avoid extra network round trips as Tom mentioned.
After some test, I find we also should set the extra_float_digits to avoid
precision lossing.
Please consider the v4 patch to review.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e2a76ba055..7ae0c7bff2 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2223,6 +2223,24 @@ retry1:
{
am_walsender = true;
am_db_walsender = true;
+
+ /*
+ * Force assorted GUC parameters to settings that ensure
+ * that we'll output data values in a form that is
+ * unambiguous to the walreceiver.
+ */
+ port->guc_options = lappend(port->guc_options,
+pstrdup("datestyle"));
+ port->guc_options = lappend(port->guc_options,
+pstrdup("ISO"));
+ port->guc_options = lappend(port->guc_options,
+pstrdup("intervalstyle"));
+ port->guc_options = lappend(port->guc_options,
+pstrdup("postgres"));
+ port->guc_options = lappend(port->guc_options,
+pstrdup("extra_float_digits"));
+ port->guc_options = lappend(port->guc_options,
+pstrdup("3"));
}
else if (!parse_bool(valptr, &am_walsender))
ereport(FATAL,
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index baa4a90771..d92ab60d86 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 5;
+use Test::More tests => 7;
# Bug #15114
@@ -224,3 +224,69 @@ $node_sub->safe_psql('postgres', "DROP TABLE tab1");
$node_pub->stop('fast');
$node_pub_sub->stop('fast');
$node_sub->stop('fast');
+
+# Verify different datestyle between publisher and subscriber.
+$node_publisher = PostgresNode->new('datestyle_publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+ "datestyle = 'SQL, MDY'");
+$node_publisher->append_conf('postgresql.conf',
+ "extra_float_digits = '-4'");
+$node_publisher->start;
+
+$node_subscriber = PostgresNode->new('datestyle_subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->append_conf('postgresql.conf',
+ "datestyle = 'SQL, DMY'");
+$node_subscriber->start;
+
+# Table for datestyle
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+
+# Table for extra_float_digits
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE flt_rep(a real, d double precision)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE flt_rep(a real, d double precision)");
+
+# Setup logical replication
+my $node_publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tab_pub FOR ALL TABLES");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tab_sub CONNECTION '$node_publisher_connstr' PUBLICATION tab_pub");
+
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_rep VALUES ('07-18-2021'), ('05-15-2018')");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM tab_rep");
+is($result, qq(2), 'failed to replication date from different datestyle');
+
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO flt_rep VALUES (1.2121323, 32.32321232132434)");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+$result = $node_subscriber-&g
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, 21 Oct 2021 at 14:04, Dilip Kumar wrote:
> On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada
> wrote:
>>
>> On Wed, Oct 20, 2021 at 8:12 PM Japin Li wrote:
>> >
>> >
>> > On Mon, 18 Oct 2021 at 17:27, Dilip Kumar wrote:
>> > > On Mon, Oct 18, 2021 at 1:41 PM Japin Li wrote:
>> > >
>> > >> I attached v3 patch that set IntervalStyle to 'postgres' when the
>> > >> server backend is walsender, and this problem has gone.
>> > >
>> > >> I test that set IntervalStyle to 'sql_standard' on publisher and
>> > >> 'iso_8601' on subscriber, it works fine.
>> > >
>> > >> Please try v3 patch and let me know if they work as unexpected.
>> > >> Thanks in advance.
>> > >
>> > > I think the idea of setting the standard DateStyle and the
>> > > IntervalStyle on the walsender process looks fine to me. As this will
>> > > avoid extra network round trips as Tom mentioned.
>> >
>> > After some test, I find we also should set the extra_float_digits to avoid
>> > precision lossing.
>>
>> Thank you for the patch!
>>
>> --- a/src/backend/postmaster/postmaster.c
>> +++ b/src/backend/postmaster/postmaster.c
>> @@ -2223,6 +2223,24 @@ retry1:
>> {
>> am_walsender = true;
>> am_db_walsender = true;
>> +
>> + /*
>> +* Force assorted GUC
>> parameters to settings that ensure
>> +* that we'll output data
>> values in a form that is
>> +* unambiguous to the walreceiver.
>> +*/
>> + port->guc_options =
>> lappend(port->guc_options,
>> +
>> pstrdup("datestyle"));
>> + port->guc_options =
>> lappend(port->guc_options,
>> +
>> pstrdup("ISO"));
>> + port->guc_options =
>> lappend(port->guc_options,
>> +
>> pstrdup("intervalstyle"));
>> + port->guc_options =
>> lappend(port->guc_options,
>> +
>> pstrdup("postgres"));
>> + port->guc_options =
>> lappend(port->guc_options,
>> +
>> pstrdup("extra_float_digits"));
>> + port->guc_options =
>> lappend(port->guc_options,
>> +
>> pstrdup("3"));
>> }
>>
>> I'm concerned that it sets parameters too early since wal senders end
>> up setting the parameters regardless of logical decoding plugins. It
>> might be better to force the parameters within the plugin for logical
>> replication, pgoutput, in order to avoid affecting other plugins? On
>> the other hand, if we do so, we will need to handle table sync worker
>> cases separately since they copy data via COPY executed by the wal
>> sender process. For example, we can have table sync workers set the
>> parameters.
>
> You mean table sync worker to set over the replication connection
> right? I think that was the first solution where normal workers, as
> well as table sync workers, were setting over the replication
> connection, but Tom suggested that setting on the walsender is a
> better option as we can avoid the network round trip.
>
> If we want to set it over the replication connection then do it for
> both as Japin's first patch is doing, otherwise, I am not seeing any
> big issue in setting it early in the walsender also. I think it is
> good to let walsender always send in the standard format which can be
> understood by other node, no?
+1
I inclined to let walsender set the parameters.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, 21 Oct 2021 at 19:54, Masahiko Sawada wrote: > On Thu, Oct 21, 2021 at 3:04 PM Dilip Kumar wrote: >> >> On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada >> wrote: >> > >> > On Wed, Oct 20, 2021 at 8:12 PM Japin Li wrote: >> > > >> > > >> > > On Mon, 18 Oct 2021 at 17:27, Dilip Kumar wrote: >> > > > On Mon, Oct 18, 2021 at 1:41 PM Japin Li wrote: >> > > > >> > > >> I attached v3 patch that set IntervalStyle to 'postgres' when the >> > > >> server backend is walsender, and this problem has gone. >> > > > >> > > >> I test that set IntervalStyle to 'sql_standard' on publisher and >> > > >> 'iso_8601' on subscriber, it works fine. >> > > > >> > > >> Please try v3 patch and let me know if they work as unexpected. >> > > >> Thanks in advance. >> > > > >> > > > I think the idea of setting the standard DateStyle and the >> > > > IntervalStyle on the walsender process looks fine to me. As this will >> > > > avoid extra network round trips as Tom mentioned. >> > > >> > > After some test, I find we also should set the extra_float_digits to >> > > avoid >> > > precision lossing. >> > >> > I'm concerned that it sets parameters too early since wal senders end >> > up setting the parameters regardless of logical decoding plugins. It >> > might be better to force the parameters within the plugin for logical >> > replication, pgoutput, in order to avoid affecting other plugins? On >> > the other hand, if we do so, we will need to handle table sync worker >> > cases separately since they copy data via COPY executed by the wal >> > sender process. For example, we can have table sync workers set the >> > parameters. >> >> You mean table sync worker to set over the replication connection >> right? I think that was the first solution where normal workers, as >> well as table sync workers, were setting over the replication >> connection, but Tom suggested that setting on the walsender is a >> better option as we can avoid the network round trip. > > Right. > > BTW I think we can set the parameters from the subscriber side without > additional network round trips by specifying the "options" parameter > in the connection string, no? > Yes, we can. However, each client should be concerned the style for datestyle, IMO it is boring. >> If we want to set it over the replication connection then do it for >> both as Japin's first patch is doing, otherwise, I am not seeing any >> big issue in setting it early in the walsender also. I think it is >> good to let walsender always send in the standard format which can be >> understood by other node, no? > > Yeah, probably the change on HEAD is fine but I'm a bit concerned > about possible issues on back branches like if the user expects to get > date data in the style of DateStyle setting on the server via > pg_recvlogical, this change could break it. > How it breaks? The user also can specify the "options" to get date data in the style which they are wanted. Right? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, 21 Oct 2021 at 22:46, Tom Lane wrote: > Japin Li writes: >> On Thu, 21 Oct 2021 at 19:54, Masahiko Sawada wrote: >>> BTW I think we can set the parameters from the subscriber side without >>> additional network round trips by specifying the "options" parameter >>> in the connection string, no? > >> Yes, we can. However, each client should be concerned the style for >> datestyle, IMO it is boring. > > There's another issue here: the subscriber can run user-defined code > (in triggers), while AFAIK the sender cannot. Sorry, I'm not sure about this. Could you give me an example? > People might be surprised > if their triggers run with a datestyle setting different from the > database's prevailing setting. So while I think it should be okay > to set-and-forget the datestyle on the sender side, we could not get > away with that in the subscriber. We'd have to set and unset for > each row, much as (e.g.) postgres_fdw has to do. > Yeah! As Masahiko said, we can avoid the network round trips by specifying the "options" parameter in the connection string. If this way is more accepted, I'll update the patch later. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, 21 Oct 2021 at 23:10, Tom Lane wrote: > Japin Li writes: >> On Thu, 21 Oct 2021 at 22:46, Tom Lane wrote: >>> There's another issue here: the subscriber can run user-defined code >>> (in triggers), while AFAIK the sender cannot. > >> Sorry, I'm not sure about this. Could you give me an example? > > If you're doing logical replication into a table that has triggers, > the replication worker has to execute those triggers. > Does that mean we should use the subscriber's settings to set the replication's parameter (e.g. datestyle)? If we do this, it might loss precision (for example: extra_float_digits on publisher is 3 and on subscriber is -4), is this accpted? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Fri, 22 Oct 2021 at 08:26, Masahiko Sawada wrote: > On Thu, Oct 21, 2021 at 11:18 PM Japin Li wrote: >> >> How it breaks? > > I don't know the real case but for example, if an application gets > changes via pg_recvlogical with a decoding plugin (say wal2json) from > the database whose DateStyle setting is "SQL, MDY", it expects that > the date values in the streamed data are in the style of "ISO, MDY". > But with this change, it will get date values in the style of "ISO" > which could lead to a parse error in the application. > >> The user also can specify the "options" to get date data >> in the style which they are wanted. Right? > > Right. But doesn't it mean breaking the compatibility? > Yeah, it might be break the compatibility. In conclusion, this bug has two ways to fix. In conclusion, this bug has two ways to fix. 1. Set the parameters on publisher, this might be break the compatibility. 2. Set the parameters on subscriber. In my first patch, I try to set the parameters after establish the connection, it will lead more network round trips. We can set the parameters when connecting the walsender using "options". For the second way, should we set the parameters same as subscriber or use the parameters (e.g. datestyle = "ISO") likes postgres_fdw set_transmission_modes()? Any thoughts? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Fri, 22 Oct 2021 at 15:00, Sadhuprasad Patro wrote:
> On Fri, Oct 22, 2021 at 8:07 AM Japin Li wrote:
>>
>>
>> On Fri, 22 Oct 2021 at 08:26, Masahiko Sawada wrote:
>> > On Thu, Oct 21, 2021 at 11:18 PM Japin Li wrote:
>> >>
>> >> How it breaks?
>> >
>> > I don't know the real case but for example, if an application gets
>> > changes via pg_recvlogical with a decoding plugin (say wal2json) from
>> > the database whose DateStyle setting is "SQL, MDY", it expects that
>> > the date values in the streamed data are in the style of "ISO, MDY".
>> > But with this change, it will get date values in the style of "ISO"
>> > which could lead to a parse error in the application.
>> >
>> >> The user also can specify the "options" to get date data
>> >> in the style which they are wanted. Right?
>> >
>> > Right. But doesn't it mean breaking the compatibility?
>> >
>>
>> Yeah, it might be break the compatibility.
>>
>> In conclusion, this bug has two ways to fix.
>>
>> In conclusion, this bug has two ways to fix.
>>
>> 1. Set the parameters on publisher, this might be break the compatibility.
>
> Is it not possible to set the parameter on publisher as "ISO, MDY" or
> "ISO, YMD", instead of only "ISO"?
> DateStyle includes both, so we may set the parameter with date format...
>
>> 2. Set the parameters on subscriber. In my first patch, I try to set the
>>parameters after establish the connection, it will lead more network
>>round trips. We can set the parameters when connecting the walsender
>>using "options".
>>
>> For the second way, should we set the parameters same as subscriber or
>> use the parameters (e.g. datestyle = "ISO") likes postgres_fdw
>> set_transmission_modes()?
>>
>> Any thoughts?
>
> IMO, setting the parameter value same as the subscriber is better. It
> is always possible that we can set any datestyle in the plugins
> itself...
>
Attach v5 patch. This patch set the datestyle, intervalstyle and
extra_float_digits parameters when we connect to publisher, this can
avoid the network round trips (compare with the first patch).
OTOH, the patch uses the subscriber's parameters as connecting parameters,
which is more complex. If we use the parameters likes postgres_fdw
set_transmission_mode(), the code will be easier [1].
[1]
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 5c6e56a5b2..0d03edd39f 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -128,8 +128,8 @@ libpqrcv_connect(const char *conninfo, bool logical, const
char *appname,
{
WalReceiverConn *conn;
PostgresPollingStatusType status;
- const char *keys[5];
- const char *vals[5];
+ const char *keys[6];
+ const char *vals[6];
int i = 0;
/*
@@ -155,6 +155,8 @@ libpqrcv_connect(const char *conninfo, bool logical, const
char *appname,
{
keys[++i] = "client_encoding";
vals[i] = GetDatabaseEncodingName();
+ keys[++i] = "options";
+ vals[i] = "-c datestyle=ISO,\\ YMD -c intervalstyle=postgres
extra_float_digits=3";
}
keys[++i] = NULL;
vals[i] = NULL;
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 5c6e56a5b2..c1fb4fd3d4 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -30,6 +30,7 @@
#include "pqexpbuffer.h"
#include "replication/walreceiver.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/pg_lsn.h"
#include "utils/tuplestore.h"
@@ -82,6 +83,8 @@ static WalRcvExecResult *libpqrcv_exec(WalReceiverConn *conn,
const int nRetTypes,
const Oid *retTypes);
static void libpqrcv_disconnect(WalReceiverConn *conn);
+static char *add_backslash(const char *value);
+static char *get_transmission_modes(void);
static WalReceiverFunctionsType PQWalReceiverFunctions = {
libpqrcv_connect,
@@ -128,9 +131,10 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
{
WalReceiverConn *conn;
PostgresPollingStatusType status;
- co
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Sat, 23 Oct 2021 at 00:55, Tom Lane wrote:
> Japin Li writes:
>> Attach v5 patch. This patch set the datestyle, intervalstyle and
>> extra_float_digits parameters when we connect to publisher, this can
>> avoid the network round trips (compare with the first patch).
>
> You could make it a little less confusing by not insisting on a
> space in the datestyle. This should work fine:
>
> vals[i] = "-c datestyle=ISO,YMD -c intervalstyle=postgres
> extra_float_digits=3";
>
Oh. My apologies. I try this style before, but find it see "ISO," is not valid,
so I add backslash, but it seems like that is my environment doesn't cleanup.
Fixed.
> Also, I think some comments would be appropriate.
>
Add comments for it.
> I don't see any value whatsoever in the more complicated version
> of the patch. It's just more code to maintain and more things
> to go wrong. And not only at our level, but the DBA's too.
Agreed.
> What if the subscriber and publisher are of different PG versions
> and have different ideas of the valid values of these settings?
>
Sorry, I'm a bit confused. Do you mean we should provide a choose for user
to set thoses parameters when establish logical replication?
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 5c6e56a5b2..2c1a598300 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -128,8 +128,8 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
{
WalReceiverConn *conn;
PostgresPollingStatusType status;
- const char *keys[5];
- const char *vals[5];
+ const char *keys[6];
+ const char *vals[6];
int i = 0;
/*
@@ -155,6 +155,13 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
{
keys[++i] = "client_encoding";
vals[i] = GetDatabaseEncodingName();
+
+ /*
+ * Force assorted GUC parameters to settings that ensure that we'll output
+ * data values in a form that is unambiguous to the publisher.
+ */
+ keys[++i] = "options";
+ vals[i] = "-c datestyle=ISO,YMD -c intervalstyle=postgres -c extra_float_digits=3";
}
keys[++i] = NULL;
vals[i] = NULL;
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index baa4a90771..d92ab60d86 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 5;
+use Test::More tests => 7;
# Bug #15114
@@ -224,3 +224,69 @@ $node_sub->safe_psql('postgres', "DROP TABLE tab1");
$node_pub->stop('fast');
$node_pub_sub->stop('fast');
$node_sub->stop('fast');
+
+# Verify different datestyle between publisher and subscriber.
+$node_publisher = PostgresNode->new('datestyle_publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+ "datestyle = 'SQL, MDY'");
+$node_publisher->append_conf('postgresql.conf',
+ "extra_float_digits = '-4'");
+$node_publisher->start;
+
+$node_subscriber = PostgresNode->new('datestyle_subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->append_conf('postgresql.conf',
+ "datestyle = 'SQL, DMY'");
+$node_subscriber->start;
+
+# Table for datestyle
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+
+# Table for extra_float_digits
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE flt_rep(a real, d double precision)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE flt_rep(a real, d double precision)");
+
+# Setup logical replication
+my $node_publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tab_pub FOR ALL TABLES");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tab_sub CONNECTION '$node_publisher_connstr' PUBLICATION tab_pub");
+
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_rep VALUES ('07-18-2021'), ('05-15-2018')");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+my $result = $node_subscriber->safe_psql(
Re: [PATCH v2] src/port/snprintf.c: Optimize the common base=10 case in fmtint
On Wed, 27 Oct 2021 at 04:58, Arjan van de Ven wrote:
> [PATCH v2] src/port/snprintf.c: Optimize the common base=10 case in fmtint
>
> fmtint() turns an integer into a string for a given base, and to do this
> it does a divide/modulo operation iteratively.
> The only possible base values are 8, 10 and 16
>
> On just about any CPU, divides are a pretty expensive operation, generally
> 10x to 20x or more expensive than adds or multiplies.
>
> By special casing the base values, the compiler (gcc or other) can (and will)
> replace the divide by a multiply with 0xcccd (for base 10) or
> bitops
> for base 8 and 16, yielding a lot faster code.
>
> I considered a switch statement, but since base 10 is the most common by far,
> I implemented it as a series of if/else statements with a likely() marking
> the 10 case.
>
> Even though this only shows up in the database creation phase of pgbench and
> not so much
> during the normal run time, the optimization is simple and high value enough
> that
> in my opinion it's worth doing
>
>
+ if (likely(base == 10)) {
+ do
+ {
+ convert[sizeof(convert) - (++vallen)] =
cvt[uvalue % 10];
+ uvalue = uvalue / 10;
+ } while (uvalue);
+ } else if (base == 16) {
Why do we need likely() for base=10, however, base=16 and base=8 don't need?
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Sat, 23 Oct 2021 at 02:00, Tom Lane wrote: > Japin Li writes: >> On Sat, 23 Oct 2021 at 00:55, Tom Lane wrote: >>> What if the subscriber and publisher are of different PG versions >>> and have different ideas of the valid values of these settings? > >> Sorry, I'm a bit confused. Do you mean we should provide a choose for user >> to set thoses parameters when establish logical replication? > > No, I'm just pointing out that pushing the subscriber's settings > to the publisher wouldn't be guaranteed to work. As long as we > use curated values that we know do what we want on all versions, > I think we're okay. > Thanks for your clarification. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Inconsistent error message for varchar(n)
Hi, hackers When I try to create table that has a varchar(n) data type, I find an inconsistent error message for it. postgres=# CREATE TABLE tbl (s varchar(2147483647)); ERROR: length for type varchar cannot exceed 10485760 LINE 1: CREATE TABLE tbl (s varchar(2147483647)); ^ postgres=# CREATE TABLE tbl (s varchar(2147483648)); ERROR: syntax error at or near "2147483648" LINE 1: CREATE TABLE tbl (s varchar(2147483648)); ^ I find that in gram.y the varchar has an integer parameter which means its value don't exceed 2147483647. The first error message is reported by anychar_typmodin(), and the later is reported by gram.y. IMO, the syntax error for varchar(n) is more confused. Any thoughts? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Invalid Unicode escape value at or near "\u0000"
Hi, hackers When I try to insert an Unicode "\u", there is an error $subject. postgres=# CREATE TABLE tbl (s varchar(10)); CREATE TABLE postgres=# INSERT INTO tbl VALUES (E'\u'); ERROR: invalid Unicode escape value at or near "\u" LINE 1: INSERT INTO tbl VALUES (E'\u'); ^ "\u" is valid unicode [1], why not we cannot insert it? [1] https://www.unicode.org/charts/PDF/U.pdf -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Invalid Unicode escape value at or near "\u0000"
On Sat, 13 Nov 2021 at 21:52, Andrew Dunstan wrote: > On 11/13/21 00:40, Thomas Munro wrote: >> On Sat, Nov 13, 2021 at 4:32 PM Japin Li wrote: >>> When I try to insert an Unicode "\u", there is an error $subject. >>> >>> postgres=# CREATE TABLE tbl (s varchar(10)); >>> CREATE TABLE >>> postgres=# INSERT INTO tbl VALUES (E'\u'); >>> ERROR: invalid Unicode escape value at or near "\u" >>> LINE 1: INSERT INTO tbl VALUES (E'\u'); >>> ^ >>> >>> "\u" is valid unicode [1], why not we cannot insert it? >> Yes, it is a valid codepoint, but unfortunately PostgreSQL can't >> support it because it sometimes deals in null terminated string, even >> though internally it does track string data and length separately. We >> have to do that to use libc facilities like strcoll_r(), and probably >> many other things. >> >> > > And it's documented at > <https://www.postgresql.org/docs/current/datatype-character.html>: > > > The characters that can be stored in any of these data types are > determined by the database character set, which is selected when the > database is created. Regardless of the specific character set, the > character with code zero (sometimes called NUL) cannot be stored. > > Thanks Thomas and Andrew. Sorry for my ignore reading of the documentation. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Inconsistent error message for varchar(n)
On Sat, 13 Nov 2021 at 23:42, Tom Lane wrote: > Japin Li writes: >> postgres=# CREATE TABLE tbl (s varchar(2147483647)); >> ERROR: length for type varchar cannot exceed 10485760 >> LINE 1: CREATE TABLE tbl (s varchar(2147483647)); >> ^ > >> postgres=# CREATE TABLE tbl (s varchar(2147483648)); >> ERROR: syntax error at or near "2147483648" >> LINE 1: CREATE TABLE tbl (s varchar(2147483648)); >> ^ > > I'm having a very hard time getting excited about that. We could maybe > switch the grammar production to use generic expr_list syntax for the > typmod, like GenericType does. But that would just result in this: > > regression=# CREATE TABLE tbl (s "varchar"(2147483648)); > ERROR: value "2147483648" is out of range for type integer > LINE 1: CREATE TABLE tbl (s "varchar"(2147483648)); > ^ > > which doesn't seem any less confusing for a novice who doesn't know > that typmods are constrained to be integers. > > There might be something to be said for switching all the hard-wired > type productions to use opt_type_modifiers and pushing the knowledge > that's in, eg, opt_float out to per-type typmodin routines. But any > benefit would be in reduction of the grammar size, and I'm dubious > that it'd be worth the trouble. I suspect that overall, the resulting > error messages would be slightly worse not better --- note for example > the poorer placement of the error cursor above. A related example is > > regression=# CREATE TABLE tbl (s varchar(2,3)); > ERROR: syntax error at or near "," > LINE 1: CREATE TABLE tbl (s varchar(2,3)); > ^ > regression=# CREATE TABLE tbl (s "varchar"(2,3)); > ERROR: invalid type modifier > LINE 1: CREATE TABLE tbl (s "varchar"(2,3)); > ^ > > That's explained by the comment in anychar_typmodin: > >* we're not too tense about good error message here because grammar >* shouldn't allow wrong number of modifiers for CHAR > > and we could surely improve that message, but anychar_typmodin can't give > a really on-point error cursor. > Oh! I didn't consider this situation. Since the max size of varchar cannot exceed 10485760, however, I cannot find this in documentation [1]. Is there something I missed? Should we mention this in the documentation? [1] https://www.postgresql.org/docs/devel/datatype-character.html -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Minor documentation fix - missing blank space
Hi, When I read the documentation about Overview of PostgreSQL Internals - Executor [1], I find there is a missing space in the documentation. diff --git a/doc/src/sgml/arch-dev.sgml b/doc/src/sgml/arch-dev.sgml index 7aff059e82..c2be28fac8 100644 --- a/doc/src/sgml/arch-dev.sgml +++ b/doc/src/sgml/arch-dev.sgml @@ -559,7 +559,7 @@ A simple INSERT ... VALUES command creates a trivial plan tree consisting of a single Result node, which computes just one result row, feeding that up -toModifyTable to perform the insertion. +to ModifyTable to perform the insertion. [1] https://www.postgresql.org/docs/14/executor.html -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: pg_replslotdata - a tool for displaying replication slot information
On Wed, 24 Nov 2021 at 23:59, Bharath Rupireddy wrote: > On Wed, Nov 24, 2021 at 9:09 PM Bharath Rupireddy >> > Thoughts? >> >> Attaching the rebased v2 patch. > > On windows the previous patches were failing, fixed that in the v3 > patch. I'm really sorry for the noise. > Cool! When I try to use it, there is an error for -v, --verbose option. px@ubuntu:~/Codes/postgres/Debug$ pg_replslotdata -v pg_replslotdata: invalid option -- 'v' Try "pg_replslotdata --help" for more information. This is because the getopt_long() missing 'v' in the third parameter. while ((c = getopt_long(argc, argv, "D:v", long_options, NULL)) != -1) -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Add test module for Table Access Method
On Tue, 16 Jan 2024 at 13:15, Bharath Rupireddy wrote: > On Tue, Jan 16, 2024 at 10:28 AM Michael Paquier wrote: >> >> Hmm. I'd rather have it do something useful in terms of test coverage >> rather than being just an empty skull. >> >> How about adding the same kind of coverage as dummy_index_am with a >> couple of reloptions then? That can serve as a point of reference >> when a table AM needs a few custom options. A second idea would be to >> show how to use toast relations when implementing your new AM, where a >> toast table could be created even in cases where we did not want one >> with heap, when it comes to size limitations with char and/or varchar, >> and that makes for a simpler needs_toast_table callback. > > I think a test module for a table AM will really help developers. Just > to add to the above list - how about the table AM implementing a > simple in-memory (columnar if possible) database storing tables > in-memory and subsequently providing readers with the access to the > tables? That's a good idea.
Introduce a new API for TableAmRoutine
Hi, hackers Recently, I'm trying to implement a new TAM for PostgreSQL, I find there is no API for handling table's option. For example: CREATE TABLE t (...) USING new_am WITH (...); Is it possible add a new API to handle table's option in TableAmRoutine? -- Regrads, Japin Li.
Unnecessary smgropen in {heapam_relation,index}_copy_data?
Hi, hackers
I find heapam_relation_copy_data() and index_copy_data() have the following
code:
dstrel = smgropen(*newrlocator, rel->rd_backend);
...
RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
The smgropen() is also called by RelationCreateStorage(), why should we call
smgropen() explicitly here?
I try to remove the smgropen(), and all tests passed.
>From 88a6670dcff67036014fd6b31062bcab5daed30e Mon Sep 17 00:00:00 2001
From: japinli
Date: Tue, 23 Jan 2024 12:34:18 +0800
Subject: [PATCH v1 1/1] Remove unnecessary smgropen
---
src/backend/access/heap/heapam_handler.c | 4 +---
src/backend/commands/tablecmds.c | 4 +---
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 5a17112c91..547fdef26a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -637,8 +637,6 @@ heapam_relation_copy_data(Relation rel, const RelFileLocator *newrlocator)
{
SMgrRelation dstrel;
- dstrel = smgropen(*newrlocator, rel->rd_backend);
-
/*
* Since we copy the file directly without looking at the shared buffers,
* we'd better first flush out any pages of the source relation that are
@@ -654,7 +652,7 @@ heapam_relation_copy_data(Relation rel, const RelFileLocator *newrlocator)
* NOTE: any conflict in relfilenumber value will be caught in
* RelationCreateStorage().
*/
- RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
+ dstrel = RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
/* copy main fork */
RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index da705ae468..5ca6277ee0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -14787,8 +14787,6 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
{
SMgrRelation dstrel;
- dstrel = smgropen(newrlocator, rel->rd_backend);
-
/*
* Since we copy the file directly without looking at the shared buffers,
* we'd better first flush out any pages of the source relation that are
@@ -14804,7 +14802,7 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
* NOTE: any conflict in relfilenumber value will be caught in
* RelationCreateStorage().
*/
- RelationCreateStorage(newrlocator, rel->rd_rel->relpersistence, true);
+ dstrel = RelationCreateStorage(newrlocator, rel->rd_rel->relpersistence, true);
/* copy main fork */
RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM,
base-commit: 80bece312c4b957ea5a93db84be1d1776f0e5e67
--
2.34.1
Re: Unnecessary smgropen in {heapam_relation,index}_copy_data?
On Thu, 25 Jan 2024 at 21:43, Aleksander Alekseev wrote: > Hi, > >> I find heapam_relation_copy_data() and index_copy_data() have the following >> code: >> >> dstrel = smgropen(*newrlocator, rel->rd_backend); >> >> ... >> >> RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, >> true); >> >> The smgropen() is also called by RelationCreateStorage(), why should we call >> smgropen() explicitly here? >> >> I try to remove the smgropen(), and all tests passed. > > That's a very good question. Note that the second argument of > smgropen() used to create dstrel changes after applying your patch. > I'm not 100% sure whether this is significant or not. > Thanks for the review. According the comments of RelationData->rd_backend, it is the backend id, if the relation is temporary. The differnece is RelationCreateStorage() uses relpersistence to determinate the backend id. > I added your patch to the nearest open commitfest so that we will not lose it: > > https://commitfest.postgresql.org/47/4794/ Thank you.
Re: Transaction timeout
On Fri, 26 Jan 2024 at 14:44, Andrey M. Borodin wrote: >> On 22 Jan 2024, at 11:23, Peter Smith wrote: >> >> Hi, This patch has a CF status of "Needs Review" [1], but it seems >> there was a CFbot test failure last time it was run [2]. Please have a >> look and post an updated version if necessary. > Thanks Peter! > Thanks for updating the patch. Here are some comments for v24. + +Terminate any session that spans longer than the specified amount of +time in transaction. The limit applies both to explicit transactions +(started with BEGIN) and to implicitly started +transaction corresponding to single statement. But this limit is not +applied to prepared transactions. +If this value is specified without units, it is taken as milliseconds. +A value of zero (the default) disables the timeout. + The sentence "But this limit is not applied to prepared transactions" is redundant, since we have a paragraph to describe this later. + + +If transaction_timeout is shorter than +idle_in_transaction_session_timeout or statement_timeout +transaction_timeout will invalidate longer timeout. + + Since we are already try to disable the timeouts, should we try to disable them even if they are equal. + + +Prepared transactions are not subject for this timeout. + Maybe wrap this with is a good idea. > I’ve inspected CI fails and they were caused by two different problems: > 1. It’s unsafe for isaoltion tester to await transaction_timeout within a > query. Usually it gets > FATAL: terminating connection due to transaction timeout > But if VM is a bit slow it can get occasional > PQconsumeInput failed: server closed the connection unexpectedly > So, currently all tests use “passive waiting”, in a session that will not > timeout. > > 2. In some cases pg_sleep(0.1) were sleeping up to 200 ms. That was making s7 > and s8 fail, because they rely on this margin. I'm curious why this happened. > I’ve separated these tests into different test timeouts-long and increased > margin to 300ms. Now tests run horrible 2431 ms. Moreover I’m afraid that on > buildfarm we can have much randomly-slower machines so this test might be > excluded. > This test checks COMMIT AND CHAIN and flow of small queries (Nik’s case). > > Also I’ve verified that every "enable_timeout_after(TRANSACTION_TIMEOUT)” and > “disable_timeout(TRANSACTION_TIMEOUT)” is necessary and found that case of > aborting "idle in transaction (aborted)” is not covered by tests. I’m not > sure we need a test for this. I see there is a test about idle_in_transaction_timeout and transaction_timeout. Both of them only check the session, but don't check the reason, so we cannot distinguish the reason they are terminated. Right? > Japin, Junwang, what do you think? However, checking the reason on the timeout session may cause regression test failed (as you point in 1), I don't strongly insist on it. -- Best regards, Japin Li.
Re: Transaction timeout
On Tue, 30 Jan 2024 at 14:22, Andrey M. Borodin wrote: >> On 26 Jan 2024, at 19:58, Japin Li wrote: >> >> Thanks for updating the patch. Here are some comments for v24. >> >> + >> +Terminate any session that spans longer than the specified amount of >> +time in transaction. The limit applies both to explicit transactions >> +(started with BEGIN) and to implicitly started >> +transaction corresponding to single statement. But this limit is not >> +applied to prepared transactions. >> +If this value is specified without units, it is taken as >> milliseconds. >> +A value of zero (the default) disables the timeout. >> + >> The sentence "But this limit is not applied to prepared transactions" is >> redundant, >> since we have a paragraph to describe this later. > Fixed. >> >> + >> + >> +If transaction_timeout is shorter than >> +idle_in_transaction_session_timeout or >> statement_timeout >> +transaction_timeout will invalidate longer >> timeout. >> + >> + >> >> Since we are already try to disable the timeouts, should we try to disable >> them even if they are equal. > > Well, we disable timeouts on equality. Fixed docs. > >> >> + >> + >> +Prepared transactions are not subject for this timeout. >> + >> >> Maybe wrap this with is a good idea. > Done. > Thanks for updating the patch. LGTM. If there is no other objections, I'll change it to ready for committer next Monday.
Inaccurate comment for pg_get_partkeydef
PSA patch to fix a comment inaccurate.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 6dc117dea8..bcb493b56c 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1855,7 +1855,7 @@ pg_get_statisticsobjdef_expressions(PG_FUNCTION_ARGS)
*
* Returns the partition key specification, ie, the following:
*
- * PARTITION BY { RANGE | LIST | HASH } (column opt_collation opt_opclass [, ...])
+ * { RANGE | LIST | HASH } (column opt_collation opt_opclass [, ...])
*/
Datum
pg_get_partkeydef(PG_FUNCTION_ARGS)
Re: Table AM Interface Enhancements
On Tue, 19 Mar 2024 at 21:05, Alexander Korotkov wrote: > Hi, Pavel! > > On Tue, Mar 19, 2024 at 11:34 AM Pavel Borisov wrote: >> On Tue, 19 Mar 2024 at 03:34, Alexander Korotkov >> wrote: >>> >>> On Sun, Mar 3, 2024 at 1:50 PM Alexander Korotkov >>> wrote: >>> > On Mon, Nov 27, 2023 at 10:18 PM Mark Dilger >>> > wrote: >>> > > >>> > > > On Nov 25, 2023, at 9:47 AM, Alexander Korotkov >>> > > > wrote: >>> > > > >>> > > >> Should the patch at least document which parts of the EState are >>> > > >> expected to be in which states, and which parts should be viewed as >>> > > >> undefined? If the implementors of table AMs rely on any/all aspects >>> > > >> of EState, doesn't that prevent future changes to how that structure >>> > > >> is used? >>> > > > >>> > > > New tuple tuple_insert_with_arbiter() table AM API method needs EState >>> > > > argument to call executor functions: ExecCheckIndexConstraints(), >>> > > > ExecUpdateLockMode(), and ExecInsertIndexTuples(). I think we >>> > > > probably need to invent some opaque way to call this executor function >>> > > > without revealing EState to table AM. Do you think this could work? >>> > > >>> > > We're clearly not accessing all of the EState, just some specific >>> > > fields, such as es_per_tuple_exprcontext. I think you could at least >>> > > refactor to pass the minimum amount of state information through the >>> > > table AM API. >>> > >>> > Yes, the table AM doesn't need the full EState, just the ability to do >>> > specific manipulation with tuples. I'll refactor the patch to make a >>> > better isolation for this. >>> >>> Please find the revised patchset attached. The changes are following: >>> 1. Patchset is rebase. to the current master. >>> 2. Patchset is reordered. I tried to put less debatable patches to the top. >>> 3. tuple_is_current() method is moved from the Table AM API to the >>> slot as proposed by Matthias van de Meent. >>> 4. Assert added to the table_free_rd_amcache() as proposed by Pavel Borisov. >> >> >> Patches 0001-0002 are unchanged compared to the last version in thread [1]. >> In my opinion, it's still ready to be committed, which was not done for time >> were too close to feature freeze one year ago. >> >> 0003 - Assert added from previous version. I still have a strong opinion >> that allowing multi-chunked data structures instead of single chunks is >> completely safe and makes natural process of Postgres improvement that is >> self-justified. The patch is simple enough and ready to be pushed. >> >> 0004 (previously 0007) - Have not changed, and there is consensus that this >> is reasonable. I've re-checked the current code. Looks safe considering >> returning a different slot, which I doubted before. So consider this patch >> also ready. >> >> 0005 (previously 0004) - Unused argument in the is_current_xact_tuple() >> signature is removed. Also comparing to v1 the code shifted from tableam >> methods to TTS's level. >> >> I'd propose to remove Assert(!TTS_EMPTY(slot)) for >> tts_minimal_is_current_xact_tuple() and tts_virtual_is_current_xact_tuple() >> as these are only error reporting functions that don't use slot actually. >> >> Comment similar to: >> +/* >> + * VirtualTupleTableSlots never have a storage tuples. We generally >> + * shouldn't get here, but provide a user-friendly message if we do. >> + */ >> also applies to tts_minimal_is_current_xact_tuple() >> >> I'd propose changes for clarity of this comment: >> %s/a storage tuples/storage tuples/g >> %s/generally//g >> >> Otherwise patch 0005 also looks good to me. >> >> I'm planning to review the remaining patches. Meanwhile think pushing what >> is now ready and uncontroversial is a good intention. >> Thank you for the work done on this patchset! > > Thank you, Pavel! > > Regarding 0005, I did apply "a storage tuples" grammar fix. Regarding > the rest of the things, I'd like to keep methods > tts_*_is_current_xact_tuple() to be similar to nearby > tts_*_getsysattr(). This is why I'm keeping the rest unchanged. I > think we could refactor that later, but together with > tts_*_getsysattr() methods. > > I'm going to push 0003, 0004 and 0005 if there are no objections. > > And I'll update 0001 and 0002 in their dedicated thread. > When I try to test the patch on Ubuntu 22.04 with GCC 11.4.0. There are some warnings as following: /home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c: In function ‘heapam_acquire_sample_rows’: /home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1603:28: warning: implicit declaration of function ‘get_tablespace_maintenance_io_concurrency’ [-Wimplicit-function-declaration] 1603 | prefetch_maximum = get_tablespace_maintenance_io_concurrency(onerel->rd_rel->reltablespace); |^ /home/japin/Codes/postgres/build/../src/backend/access/heap/heapam_handler.c:1757:30: war
Cannot find a working 64-bit integer type on Illumos
Hi, hackers, When I try to configure PostgreSQL 16.2 on Illumos using the following command, it complains $subject. ./configure --enable-cassert --enable-debug --enable-nls --with-perl \ --with-python --without-tcl --without-gssapi --with-openssl \ --with-ldap --with-libxml --with-libxslt --without-systemd \ --with-readline --enable-thread-safety --enable-dtrace \ DTRACEFLAGS=-64 CFLAGS=-Werror However, if I remove the `CFLAGS=-Werror`, it works fine. I'm not sure what happened here. $ uname -a SunOS db_build 5.11 hunghu-20231216T132436Z i86pc i386 i86pc illumos $ gcc --version gcc (GCC) 10.4.0 Copyright (C) 2020 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Re: Cannot find a working 64-bit integer type on Illumos
On Sat, 23 Mar 2024 at 01:04, Tom Lane wrote: > Japin Li writes: >> When I try to configure PostgreSQL 16.2 on Illumos using the following >> command, >> it complains $subject. > >> ./configure --enable-cassert --enable-debug --enable-nls --with-perl \ >> --with-python --without-tcl --without-gssapi --with-openssl \ >> --with-ldap --with-libxml --with-libxslt --without-systemd \ >> --with-readline --enable-thread-safety --enable-dtrace \ >> DTRACEFLAGS=-64 CFLAGS=-Werror > >> However, if I remove the `CFLAGS=-Werror`, it works fine. >> I'm not sure what happened here. > > CFLAGS=-Werror breaks a whole lot of configure's tests, not only that > one. (We even have this documented, see [1].) So you can't inject > -Werror that way. What I do on my buildfarm animals is the equivalent > of > > export COPT='-Werror' > > after configure and before build. I think configure pays no attention > to COPT, so it'd likely be safe to keep that set all the time, but in > the buildfarm client it's just as easy to be conservative. > > regards, tom lane > > [1] https://www.postgresql.org/docs/devel/install-make.html#CONFIGURE-ENVVARS Thank you very much! I didn't notice this part before.
Re: Cannot find a working 64-bit integer type on Illumos
On Sat, 23 Mar 2024 at 01:22, Japin Li wrote:
> On Sat, 23 Mar 2024 at 01:04, Tom Lane wrote:
>> Japin Li writes:
>>> When I try to configure PostgreSQL 16.2 on Illumos using the following
>>> command,
>>> it complains $subject.
>>
>>> ./configure --enable-cassert --enable-debug --enable-nls --with-perl \
>>> --with-python --without-tcl --without-gssapi --with-openssl \
>>> --with-ldap --with-libxml --with-libxslt --without-systemd \
>>> --with-readline --enable-thread-safety --enable-dtrace \
>>> DTRACEFLAGS=-64 CFLAGS=-Werror
>>
>>> However, if I remove the `CFLAGS=-Werror`, it works fine.
>>> I'm not sure what happened here.
>>
>> CFLAGS=-Werror breaks a whole lot of configure's tests, not only that
>> one. (We even have this documented, see [1].) So you can't inject
>> -Werror that way. What I do on my buildfarm animals is the equivalent
>> of
>>
>> export COPT='-Werror'
>>
>> after configure and before build. I think configure pays no attention
>> to COPT, so it'd likely be safe to keep that set all the time, but in
>> the buildfarm client it's just as easy to be conservative.
>>
>> regards, tom lane
>>
>> [1] https://www.postgresql.org/docs/devel/install-make.html#CONFIGURE-ENVVARS
>
> Thank you very much! I didn't notice this part before.
I try to use the following to compile it, however, it cannot compile it.
$ ../configure --enable-cassert --enable-debug --enable-nls --with-perl
--with-python --without-tcl --without-gssapi --with-openssl --with-ldap
--with-libxml --with-libxslt --without-systemd --with-readline
--enable-thread-safety --enable-dtrace DTRACEFLAGS=-64
$ make COPT='-Werror' -s
/home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c: In function
'repairDependencyLoop':
/home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c:1276:3: error:
format not a string literal and no format arguments [-Werror=format-security]
1276 | pg_log_warning(ngettext("there are circular foreign-key constraints
on this table:",
| ^~
cc1: all warnings being treated as errors
make[3]: *** [: pg_dump_sort.o] Error 1
make[2]: *** [Makefile:43: all-pg_dump-recurse] Error 2
make[1]: *** [Makefile:42: all-bin-recurse] Error 2
make: *** [GNUmakefile:11: all-src-recurse] Error 2
Re: Cannot find a working 64-bit integer type on Illumos
On Mon, 25 Mar 2024 at 09:32, Tom Lane wrote:
> Japin Li writes:
>> /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c: In function
>> 'repairDependencyLoop':
>> /home/japin/postgres/debug/../src/bin/pg_dump/pg_dump_sort.c:1276:3: error:
>> format not a string literal and no format arguments [-Werror=format-security]
>> 1276 | pg_log_warning(ngettext("there are circular foreign-key
>> constraints on this table:",
>> | ^~
>
> Yeah, some of the older buildfarm animals issue that warning too.
> AFAICS it's a bogus compiler heuristic: there is not anything
> wrong with the code as given.
>
Thanks! It seems I should remove -Werror option on Illumos.
Re: Table AM Interface Enhancements
On Thu, 28 Mar 2024 at 21:26, Alexander Korotkov wrote:
> Hi Pavel!
>
> Revised patchset is attached.
>
> On Thu, Mar 28, 2024 at 3:12 PM Pavel Borisov wrote:
>> The other extensibility that seems quite clear and uncontroversial to me is
>> 0006.
>>
>> It simply shifts the decision on whether tuple inserts should invoke inserts
>> to the related indices to the table am level. It doesn't change the current
>> heap insert behavior so it's safe for the existing heap access method. But
>> new table access methods could redefine this (only for tables created with
>> these am's) and make index inserts independently of ExecInsertIndexTuples
>> inside their own implementations of tuple_insert/tuple_multi_insert methods.
>>
>> I'd propose changing the comment:
>>
>> 1405 * This function sets `*insert_indexes` to true if expects caller to
>> return
>> 1406 * the relevant index tuples. If `*insert_indexes` is set to false,
>> then
>> 1407 * this function cares about indexes itself.
>>
>> in the following way
>>
>> Tableam implementation of tuple_insert should set `*insert_indexes` to true
>> if it expects the caller to insert the relevant index tuples (as in heap
>> implementation). It should set `*insert_indexes` to false if it cares
>> about index inserts itself and doesn't want the caller to do index inserts.
>
> Changed as you proposed.
>
>> Maybe, a commit message is also better to reformulate to describe better who
>> should do what.
>
> Done.
>
> Also, I removed extra includes in 0001 as you proposed and edited the
> commit message in 0002.
>
>> I think, with rebase and correction in the comments/commit message patch
>> 0006 is ready to be committed.
>
> I'm going to push 0001, 0002 and 0006 if no objections.
Thanks for updating the patches. Here are some minor sugesstion.
0003
+static inline TupleTableSlot *
+heapam_tuple_insert_with_arbiter(ResultRelInfo *resultRelInfo,
I'm not entirely certain whether the "inline" keyword has any effect.
0004
+static bytea *
+heapam_indexoptions(amoptions_function amoptions, char relkind,
+ Datum reloptions, bool validate)
+{
+ return index_reloptions(amoptions, reloptions, validate);
+}
Could you please explain why we are not verifying the relkind like
heapam_reloptions()?
- case RELKIND_VIEW:
case RELKIND_MATVIEW:
+ case RELKIND_VIEW:
case RELKIND_PARTITIONED_TABLE:
I think this change is unnecessary.
+ {
+ Form_pg_class classForm;
+ HeapTuple classTup;
+
+ /* fetch the relation's relcache entry */
+ if (relation->rd_index->indrelid >=
FirstNormalObjectId)
+ {
+ classTup = SearchSysCacheCopy1(RELOID,
ObjectIdGetDatum(relation->rd_index->indrelid));
+ classForm = (Form_pg_class)
GETSTRUCT(classTup);
+ if (classForm->relam >=
FirstNormalObjectId)
+ tableam =
GetTableAmRoutineByAmOid(classForm->relam);
+ else
+ tableam =
GetHeapamTableAmRoutine();
+ heap_freetuple(classTup);
+ }
+ else
+ {
+ tableam = GetHeapamTableAmRoutine();
+ }
+ amoptsfn = relation->rd_indam->amoptions;
+ }
- We can reduce the indentation by moving the classFrom and classTup into
the if branch.
- Perhaps we could remove the brace of else branch to maintain consistency
in the code style.
--
Regards,
Japin Li
Fix parameters order for relation_copy_for_cluster
Hi,
When attempting to implement a new table access method, I discovered that
relation_copy_for_cluster() has the following declaration:
void(*relation_copy_for_cluster) (Relation NewTable,
Relation OldTable,
Relation OldIndex,
bool use_sort,
TransactionId OldestXmin,
TransactionId *xid_cutoff,
MultiXactId *multi_cutoff,
double *num_tuples,
double *tups_vacuumed,
double *tups_recently_dead);
It claims that the first parameter is a new table, and the second one is an
old table. However, the table_relation_copy_for_cluster() uses the first
parameter as the old table, and the second as a new table, see below:
static inline void
table_relation_copy_for_cluster(Relation OldTable, Relation NewTable,
Relation OldIndex,
bool use_sort,
TransactionId OldestXmin,
TransactionId *xid_cutoff,
MultiXactId *multi_cutoff,
double *num_tuples,
double *tups_vacuumed,
double *tups_recently_dead)
{
OldTable->rd_tableam->relation_copy_for_cluster(OldTable, NewTable,
OldIndex,
use_sort, OldestXmin,
xid_cutoff, multi_cutoff,
num_tuples, tups_vacuumed,
tups_recently_dead);
}
It's a bit confusing, so attach a patch to fix this.
--
Regards,
Japin Li
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index cf76fc29d4..0b79566758 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -631,8 +631,8 @@ typedef struct TableAmRoutine
const RelFileLocator *newrlocator);
/* See table_relation_copy_for_cluster() */
- void (*relation_copy_for_cluster) (Relation NewTable,
- Relation OldTable,
+ void (*relation_copy_for_cluster) (Relation OldTable,
+ Relation NewTable,
Relation OldIndex,
bool use_sort,
TransactionId OldestXmin,
Re: Table AM Interface Enhancements
On Tue, 02 Apr 2024 at 07:59, Alexander Korotkov wrote: > On Mon, Apr 1, 2024 at 7:36 PM Tom Lane wrote: >> Coverity complained about what you did in RelationParseRelOptions >> in c95c25f9a: >> >> *** CID 1595992: Null pointer dereferences (FORWARD_NULL) >> /srv/coverity/git/pgsql-git/postgresql/src/backend/utils/cache/relcache.c: >> 499 in RelationParseRelOptions() >> 493 >> 494 /* >> 495 * Fetch reloptions from tuple; have to use a hardwired >> descriptor because >> 496 * we might not have any other for pg_class yet (consider >> executing this >> 497 * code for pg_class itself) >> 498 */ >> >>> CID 1595992: Null pointer dereferences (FORWARD_NULL) >> >>> Passing null pointer "tableam" to "extractRelOptions", which >> >>> dereferences it. >> 499 options = extractRelOptions(tuple, GetPgClassDescriptor(), >> 500 >> tableam, amoptsfn); >> 501 >> >> I see that extractRelOptions only uses the tableam argument for some >> relkinds, and RelationParseRelOptions does set it up for those >> relkinds --- but Coverity's complaint isn't without merit, because >> those two switch statements are looking at *different copies of the >> relkind*, which in theory could be different. This all seems quite >> messy and poorly factored. Can't we do better? Why do we need to >> involve two copies of allegedly the same pg_class tuple, anyhow? > > I wasn't registered at Coverity yet. Now I've registered and am > waiting for approval to access the PostgreSQL analysis data. > > I wonder why Coverity complains about tableam, but not amoptsfn. > Their usage patterns are very similar. > > It appears that relation->rd_rel isn't the full copy of pg_class tuple > (see AllocateRelationDesc). RelationParseRelOptions() is going to > update relation->rd_options, and thus needs a full pg_class tuple to > fetch options out of it. However, it is really unnecessary to access > both tuples at the same time. We can use a full tuple, not > relation->rd_rel, in both cases. See the attached patch. > > -- > Regards, > Alexander Korotkov + Form_pg_class classForm = (Form_pg_class) GETSTRUCT(tuple); +; There is an additional semicolon in the code. -- Regards, Japin Li
Re: Thoughts about NUM_BUFFER_PARTITIONS
On Mon, 19 Feb 2024 at 00:56, Tomas Vondra wrote: > On 2/18/24 03:30, Li Japin wrote: >> >> I find it seems need to change MAX_SIMUL_LWLOCKS if we enlarge the >> NUM_BUFFER_PARTITIONS, >> I didn’t find any comments to describe the relation between >> MAX_SIMUL_LWLOCKS and >> NUM_BUFFER_PARTITIONS, am I missing someghing? > > IMHO the relationship is pretty simple - MAX_SIMUL_LWLOCKS needs to be > higher than NUM_BUFFER_PARTITIONS, so that the backend can acquire all > the partition locks if needed. > Thanks for the explanation! Got it. > There's other places that acquire a bunch of locks, and all of them need > to be careful not to exceed MAX_SIMUL_LWLOCKS. For example gist has > GIST_MAX_SPLIT_PAGES. > > > regards
Re: Transaction timeout
On Mon, 19 Feb 2024 at 17:14, Andrey M. Borodin wrote: >> On 18 Feb 2024, at 22:16, Andrey M. Borodin wrote: >> >> But it seems a little strange that session 3 did not fail at all > It was only coincidence. Any test that verifies FATALing out in 100ms can > fail, see new failure here [0]. > > In a nearby thread Michael is proposing injections points that can wait and > be awaken. So I propose following course of action: > 1. Remove all tests that involve pg_stat_activity test of FATALed session > (any permutation with checker_sleep step) > 2. Add idle_in_transaction_session_timeout, statement_timeout and > transaction_timeout tests when injection points features get committed. > +1 > Alexander, what do you think? >
Re: Switching XLog source from archive to streaming when primary available
On Mon, 19 Feb 2024 at 18:36, Bharath Rupireddy
wrote:
> On Wed, Jan 31, 2024 at 6:30 PM Bharath Rupireddy
> wrote:
>>
>> Needed a rebase due to commit 776621a (conflict in
>> src/test/recovery/meson.build for new TAP test file added). Please
>> find the attached v17 patch.
>
> Strengthened tests a bit by using recovery_min_apply_delay to mimic
> standby spending some time fetching from archive. PSA v18 patch.
Here are some minor comments:
[1]
+primary). However, the standby exhausts all the WAL present in pg_wal
s|pg_wal|pg_wal|g
[2]
+# Ensure checkpoint doesn't come in our way
+$primary->append_conf('postgresql.conf', qq(
+min_wal_size = 2MB
+max_wal_size = 1GB
+checkpoint_timeout = 1h
+ autovacuum = off
+));
Keeping the same indentation might be better.
Re: Switching XLog source from archive to streaming when primary available
On Tue, 20 Feb 2024 at 13:40, Bharath Rupireddy
wrote:
> On Mon, Feb 19, 2024 at 8:25 PM Japin Li wrote:
>> [2]
>> +# Ensure checkpoint doesn't come in our way
>> +$primary->append_conf('postgresql.conf', qq(
>> +min_wal_size = 2MB
>> +max_wal_size = 1GB
>> +checkpoint_timeout = 1h
>> + autovacuum = off
>> +));
>>
>> Keeping the same indentation might be better.
>
> The autovacuum line looks mis-indented in the patch file. However, I
> now ran src/tools/pgindent/perltidyrc
> src/test/recovery/t/041_wal_source_switch.pl on it.
>
Thanks for updating the patch. It seems still with the wrong indent.
diff --git a/src/test/recovery/t/041_wal_source_switch.pl b/src/test/recovery/t/041_wal_source_switch.pl
index 082680bf4a..b5eddba1d5 100644
--- a/src/test/recovery/t/041_wal_source_switch.pl
+++ b/src/test/recovery/t/041_wal_source_switch.pl
@@ -18,9 +18,9 @@ $primary->init(
# Ensure checkpoint doesn't come in our way
$primary->append_conf(
'postgresql.conf', qq(
-min_wal_size = 2MB
-max_wal_size = 1GB
-checkpoint_timeout = 1h
+ min_wal_size = 2MB
+ max_wal_size = 1GB
+ checkpoint_timeout = 1h
autovacuum = off
));
$primary->start;
@@ -85,7 +85,7 @@ my $offset = -s $standby->logfile;
my $apply_delay = $retry_interval * 5;
$standby->append_conf(
'postgresql.conf', qq(
-recovery_min_apply_delay = '${apply_delay}ms'
+ recovery_min_apply_delay = '${apply_delay}ms'
));
$standby->start;
Re: Improve readability by using designated initializers when possible
On Mon, 26 Feb 2024 at 16:41, jian he wrote:
> Hi. minor issues.
>
> @@ -2063,12 +2009,12 @@ find_expr_references_walker(Node *node,
> CoerceViaIO *iocoerce = (CoerceViaIO *) node;
>
> /* since there is no exposed function, need to depend on type */
> - add_object_address(OCLASS_TYPE, iocoerce->resulttype, 0,
> + add_object_address(TypeRelationId iocoerce->resulttype, 0,
> context->addrs);
>
> @@ -2090,21 +2036,21 @@ find_expr_references_walker(Node *node,
> ConvertRowtypeExpr *cvt = (ConvertRowtypeExpr *) node;
>
> /* since there is no function dependency, need to depend on type */
> - add_object_address(OCLASS_TYPE, cvt->resulttype, 0,
> + add_object_address(TypeRelationId cvt->resulttype, 0,
> context->addrs);
>
> obvious typo errors.
>
> diff --git a/src/common/relpath.c b/src/common/relpath.c
> index b16fe19dea6..d9214f915c9 100644
> --- a/src/common/relpath.c
> +++ b/src/common/relpath.c
> @@ -31,10 +31,10 @@
> * pg_relation_size().
> */
> const char *const forkNames[] = {
> - "main", /* MAIN_FORKNUM */
> - "fsm", /* FSM_FORKNUM */
> - "vm", /* VISIBILITYMAP_FORKNUM */
> - "init" /* INIT_FORKNUM */
> + [MAIN_FORKNUM] = "main",
> + [FSM_FORKNUM] = "fsm",
> + [VISIBILITYMAP_FORKNUM] = "vm",
> + [INIT_FORKNUM] = "init",
> };
>
> `+ [INIT_FORKNUM] = "init", ` no need for an extra comma?
>
> + [PG_SJIS] = {0, 0, pg_sjis_mblen, pg_sjis_dsplen,
> pg_sjis_verifychar, pg_sjis_verifystr, 2},
> + [PG_BIG5] = {0, 0, pg_big5_mblen, pg_big5_dsplen,
> pg_big5_verifychar, pg_big5_verifystr, 2},
> + [PG_GBK] = {0, 0, pg_gbk_mblen, pg_gbk_dsplen, pg_gbk_verifychar,
> pg_gbk_verifystr, 2},
> + [PG_UHC] = {0, 0, pg_uhc_mblen, pg_uhc_dsplen, pg_uhc_verifychar,
> pg_uhc_verifystr, 2},
> + [PG_GB18030] = {0, 0, pg_gb18030_mblen, pg_gb18030_dsplen,
> pg_gb18030_verifychar, pg_gb18030_verifystr, 4},
> + [PG_JOHAB] = {0, 0, pg_johab_mblen, pg_johab_dsplen,
> pg_johab_verifychar, pg_johab_verifystr, 3},
> + [PG_SHIFT_JIS_2004] = {0, 0, pg_sjis_mblen, pg_sjis_dsplen,
> pg_sjis_verifychar, pg_sjis_verifystr, 2},
> };
> similarly, last entry, no need an extra comma?
> also other places last array entry no need extra comma.
For last entry comma, see [1].
[1]
https://www.postgresql.org/message-id/386f8c45-c8ac-4681-8add-e3b0852c1620%40eisentraut.org
Re: Improve readability by using designated initializers when possible
On Tue, 27 Feb 2024 at 19:55, Jelte Fennema-Nio wrote:
> On Tue, 27 Feb 2024 at 12:52, Jelte Fennema-Nio wrote:
>> Attached is an updated patchset to also convert pg_enc2icu_tbl and
>> pg_enc2gettext_tbl. I converted pg_enc2gettext_tbl in a separate
>> commit, because it actually requires some codechanges too.
>
> Another small update to also make all arrays changed by this patch
> have a trailing comma (to avoid future diff noise).
I see the config_group_names[] needs null-terminated because of help_config,
however, I didn't find the reference in help_config.c. Is this comment
outdated? Here is a patch to remove the null-terminated.
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index 59904fd007..df849f73fc 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -715,11 +715,9 @@ const char *const config_group_names[] =
[PRESET_OPTIONS] = gettext_noop("Preset Options"),
[CUSTOM_OPTIONS] = gettext_noop("Customized Options"),
[DEVELOPER_OPTIONS] = gettext_noop("Developer Options"),
- /* help_config wants this array to be null-terminated */
- NULL
};
-StaticAssertDecl(lengthof(config_group_names) == (DEVELOPER_OPTIONS + 2),
+StaticAssertDecl(lengthof(config_group_names) == (DEVELOPER_OPTIONS + 1),
"array length mismatch");
/*
Re: Improve readability by using designated initializers when possible
On Wed, 28 Feb 2024 at 00:06, Jelte Fennema-Nio wrote: > On Tue, 27 Feb 2024 at 16:04, Japin Li wrote: >> I see the config_group_names[] needs null-terminated because of help_config, >> however, I didn't find the reference in help_config.c. Is this comment >> outdated? > > Yeah, you're correct. That comment has been outdated for more than 20 > years. The commit that made it unnecessary to null-terminate the array > was 9d77708d83ee. > > Attached is v5 of the patchset that also includes this change (with > you listed as author). Thanks for updating the patch! It looks good to me except there is an outdated comment. diff --git a/src/common/encnames.c b/src/common/encnames.c index bd012fe3a0..dba6bd2c9e 100644 --- a/src/common/encnames.c +++ b/src/common/encnames.c @@ -297,7 +297,6 @@ static const pg_encname pg_encname_tbl[] = /* -- * These are "official" encoding names. - * XXX must be sorted by the same order as enum pg_enc (in mb/pg_wchar.h) * -- */ #ifndef WIN32
Re: Improve readability by using designated initializers when possible
On Mon, 04 Mar 2024 at 07:46, Michael Paquier wrote:
> On Fri, Mar 01, 2024 at 06:30:10AM +0100, Jelte Fennema-Nio wrote:
>> On Fri, 1 Mar 2024 at 06:08, Michael Paquier wrote:
>>> Mostly OK to me. Just note that this comment is incorrect because
>>> pg_enc2gettext_tbl[] includes elements in the range
>>> [PG_SJIS,_PG_LAST_ENCODING_[ ;)
>>
>> fixed
>
> (Forgot to update this thread.)
> Thanks, applied this one. I went over a few versions of the comment
> in pg_wchar.h, and tweaked it to something that was of one of the
> previous versions, I think.
Hi,
Attach a patch to rewrite dispatch_table array using C99-designated
initializer syntax.
>From f398d6d310e9436c5e415baa6fd273981a9e181f Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Tue, 5 Mar 2024 21:32:46 +0800
Subject: [PATCH v1 1/1] Use C99-designated initializer syntax for
dispatch_table array
---
src/backend/executor/execExprInterp.c | 195 +-
src/include/executor/execExpr.h | 3 -
2 files changed, 96 insertions(+), 102 deletions(-)
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 7c1f51e2e0..e4ad522312 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -400,107 +400,104 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
TupleTableSlot *outerslot;
TupleTableSlot *scanslot;
- /*
- * This array has to be in the same order as enum ExprEvalOp.
- */
#if defined(EEO_USE_COMPUTED_GOTO)
static const void *const dispatch_table[] = {
- &&CASE_EEOP_DONE,
- &&CASE_EEOP_INNER_FETCHSOME,
- &&CASE_EEOP_OUTER_FETCHSOME,
- &&CASE_EEOP_SCAN_FETCHSOME,
- &&CASE_EEOP_INNER_VAR,
- &&CASE_EEOP_OUTER_VAR,
- &&CASE_EEOP_SCAN_VAR,
- &&CASE_EEOP_INNER_SYSVAR,
- &&CASE_EEOP_OUTER_SYSVAR,
- &&CASE_EEOP_SCAN_SYSVAR,
- &&CASE_EEOP_WHOLEROW,
- &&CASE_EEOP_ASSIGN_INNER_VAR,
- &&CASE_EEOP_ASSIGN_OUTER_VAR,
- &&CASE_EEOP_ASSIGN_SCAN_VAR,
- &&CASE_EEOP_ASSIGN_TMP,
- &&CASE_EEOP_ASSIGN_TMP_MAKE_RO,
- &&CASE_EEOP_CONST,
- &&CASE_EEOP_FUNCEXPR,
- &&CASE_EEOP_FUNCEXPR_STRICT,
- &&CASE_EEOP_FUNCEXPR_FUSAGE,
- &&CASE_EEOP_FUNCEXPR_STRICT_FUSAGE,
- &&CASE_EEOP_BOOL_AND_STEP_FIRST,
- &&CASE_EEOP_BOOL_AND_STEP,
- &&CASE_EEOP_BOOL_AND_STEP_LAST,
- &&CASE_EEOP_BOOL_OR_STEP_FIRST,
- &&CASE_EEOP_BOOL_OR_STEP,
- &&CASE_EEOP_BOOL_OR_STEP_LAST,
- &&CASE_EEOP_BOOL_NOT_STEP,
- &&CASE_EEOP_QUAL,
- &&CASE_EEOP_JUMP,
- &&CASE_EEOP_JUMP_IF_NULL,
- &&CASE_EEOP_JUMP_IF_NOT_NULL,
- &&CASE_EEOP_JUMP_IF_NOT_TRUE,
- &&CASE_EEOP_NULLTEST_ISNULL,
- &&CASE_EEOP_NULLTEST_ISNOTNULL,
- &&CASE_EEOP_NULLTEST_ROWISNULL,
- &&CASE_EEOP_NULLTEST_ROWISNOTNULL,
- &&CASE_EEOP_BOOLTEST_IS_TRUE,
- &&CASE_EEOP_BOOLTEST_IS_NOT_TRUE,
- &&CASE_EEOP_BOOLTEST_IS_FALSE,
- &&CASE_EEOP_BOOLTEST_IS_NOT_FALSE,
- &&CASE_EEOP_PARAM_EXEC,
- &&CASE_EEOP_PARAM_EXTERN,
- &&CASE_EEOP_PARAM_CALLBACK,
- &&CASE_EEOP_CASE_TESTVAL,
- &&CASE_EEOP_MAKE_READONLY,
- &&CASE_EEOP_IOCOERCE,
- &&CASE_EEOP_IOCOERCE_SAFE,
- &&CASE_EEOP_DISTINCT,
- &&CASE_EEOP_NOT_DISTINCT,
- &&CASE_EEOP_NULLIF,
- &&CASE_EEOP_SQLVALUEFUNCTION,
- &&CASE_EEOP_CURRENTOFEXPR,
- &&CASE_EEOP_NEXTVALUEEXPR,
- &&CASE_EEOP_ARRAYEXPR,
- &&CASE_EEOP_ARRAYCOERCE,
- &&CASE_EEOP_ROW,
- &&CASE_EEOP_ROWCOMPARE_STEP,
- &&CASE_EEOP_ROWCOMPARE_FINAL,
- &&CASE_EEOP_MINMAX,
- &&CASE_EEOP_FIELDSELECT,
- &&CASE_EEOP_FIELDSTORE_DEFORM,
- &&CASE_EEOP_FIELDSTORE_FORM,
- &&CASE_EEOP_SBSREF_SUBSCRIPTS,
- &&CASE_EEOP_SBSREF_OLD,
- &&CASE_EEOP_SBSREF_ASSIGN,
- &&CASE_EEOP_SBSREF_FETCH,
- &&CASE_EEOP_DOMAIN_TESTVAL,
- &&CASE_EEOP_DOMAIN_NOTNULL,
- &&CASE_EEOP_DOMAIN_CHECK,
- &&CASE_EEOP_CONVERT_ROWTYPE,
- &&CASE_EEOP_SCALARARRAYOP,
- &&CASE_EEOP_HASHED_SCALARARRAYOP,
- &&CASE_EEOP_XMLEXPR,
- &&CASE_EEOP_JSON_CONSTRUCTOR,
- &&CASE_EEOP_IS_JSON,
- &&CASE_EEOP_AGGREF,
- &&CASE_EEOP_GROUPING_FUNC,
- &&CASE_EEOP_WINDOW_FUNC,
- &&CASE_EEOP_SUBPLAN,
- &&CASE_EEOP_AGG_STRICT_DESERIALIZE,
- &&CASE_EEOP_AGG_DESERIALIZE,
- &&CASE_EEOP_AGG_STRICT_INPUT_CHECK_ARGS,
- &&CASE_EEOP_AGG_STRICT_INPUT_CHECK_NULLS,
- &&CASE_EEOP_AGG_PLAIN_PERGROUP_NULLCHECK,
- &&CASE_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYVAL,
- &&am
Re: Improve readability by using designated initializers when possible
On Tue, 05 Mar 2024 at 22:03, Jelte Fennema-Nio wrote:
> On Tue, 5 Mar 2024 at 14:50, Japin Li wrote:
>> Attach a patch to rewrite dispatch_table array using C99-designated
>> initializer syntax.
>
> Looks good. Two small things:
Thanks for the review.
>
> + [EEOP_LAST] = &&CASE_EEOP_LAST,
>
> Is EEOP_LAST actually needed in this array? It seems unused afaict. If
> indeed not needed, that would be good to remove in an additional
> commit.
There is a warning if remove it, so I keep it.
/home/japin/Codes/postgres/build/../src/backend/executor/execExprInterp.c:118:33:
warning: label ‘CASE_EEOP_LAST’ defined but not used [-Wunused-label]
118 | #define EEO_CASE(name) CASE_##name:
| ^
/home/japin/Codes/postgres/build/../src/backend/executor/execExprInterp.c:1845:17:
note: in expansion of macro ‘EEO_CASE’
1845 | EEO_CASE(EEOP_LAST)
| ^~~~
>
> - *
> - * The order of entries needs to be kept in sync with the dispatch_table[]
> - * array in execExprInterp.c:ExecInterpExpr().
>
> I think it would be good to at least keep the comment saying that this
> array should be updated (only the order doesn't need to be strictly
> kept in sync anymore).
Fixed.
>From 20a72f2a5e0b282812ecde5c6aef2ce4ab118003 Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Tue, 5 Mar 2024 21:32:46 +0800
Subject: [PATCH v2 1/1] Use C99-designated initializer syntax for
dispatch_table array
---
src/backend/executor/execExprInterp.c | 195 +-
src/include/executor/execExpr.h | 2 +-
2 files changed, 97 insertions(+), 100 deletions(-)
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 7c1f51e2e0..e4ad522312 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -400,107 +400,104 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
TupleTableSlot *outerslot;
TupleTableSlot *scanslot;
- /*
- * This array has to be in the same order as enum ExprEvalOp.
- */
#if defined(EEO_USE_COMPUTED_GOTO)
static const void *const dispatch_table[] = {
- &&CASE_EEOP_DONE,
- &&CASE_EEOP_INNER_FETCHSOME,
- &&CASE_EEOP_OUTER_FETCHSOME,
- &&CASE_EEOP_SCAN_FETCHSOME,
- &&CASE_EEOP_INNER_VAR,
- &&CASE_EEOP_OUTER_VAR,
- &&CASE_EEOP_SCAN_VAR,
- &&CASE_EEOP_INNER_SYSVAR,
- &&CASE_EEOP_OUTER_SYSVAR,
- &&CASE_EEOP_SCAN_SYSVAR,
- &&CASE_EEOP_WHOLEROW,
- &&CASE_EEOP_ASSIGN_INNER_VAR,
- &&CASE_EEOP_ASSIGN_OUTER_VAR,
- &&CASE_EEOP_ASSIGN_SCAN_VAR,
- &&CASE_EEOP_ASSIGN_TMP,
- &&CASE_EEOP_ASSIGN_TMP_MAKE_RO,
- &&CASE_EEOP_CONST,
- &&CASE_EEOP_FUNCEXPR,
- &&CASE_EEOP_FUNCEXPR_STRICT,
- &&CASE_EEOP_FUNCEXPR_FUSAGE,
- &&CASE_EEOP_FUNCEXPR_STRICT_FUSAGE,
- &&CASE_EEOP_BOOL_AND_STEP_FIRST,
- &&CASE_EEOP_BOOL_AND_STEP,
- &&CASE_EEOP_BOOL_AND_STEP_LAST,
- &&CASE_EEOP_BOOL_OR_STEP_FIRST,
- &&CASE_EEOP_BOOL_OR_STEP,
- &&CASE_EEOP_BOOL_OR_STEP_LAST,
- &&CASE_EEOP_BOOL_NOT_STEP,
- &&CASE_EEOP_QUAL,
- &&CASE_EEOP_JUMP,
- &&CASE_EEOP_JUMP_IF_NULL,
- &&CASE_EEOP_JUMP_IF_NOT_NULL,
- &&CASE_EEOP_JUMP_IF_NOT_TRUE,
- &&CASE_EEOP_NULLTEST_ISNULL,
- &&CASE_EEOP_NULLTEST_ISNOTNULL,
- &&CASE_EEOP_NULLTEST_ROWISNULL,
- &&CASE_EEOP_NULLTEST_ROWISNOTNULL,
- &&CASE_EEOP_BOOLTEST_IS_TRUE,
- &&CASE_EEOP_BOOLTEST_IS_NOT_TRUE,
- &&CASE_EEOP_BOOLTEST_IS_FALSE,
- &&CASE_EEOP_BOOLTEST_IS_NOT_FALSE,
- &&CASE_EEOP_PARAM_EXEC,
- &&CASE_EEOP_PARAM_EXTERN,
- &&CASE_EEOP_PARAM_CALLBACK,
- &&CASE_EEOP_CASE_TESTVAL,
- &&CASE_EEOP_MAKE_READONLY,
- &&CASE_EEOP_IOCOERCE,
- &&CASE_EEOP_IOCOERCE_SAFE,
- &&CASE_EEOP_DISTINCT,
- &&CASE_EEOP_NOT_DISTINCT,
- &&CASE_EEOP_NULLIF,
- &&CASE_EEOP_SQLVALUEFUNCTION,
- &&CASE_EEOP_CURRENTOFEXPR,
- &&CASE_EEOP_NEXTVALUEEXPR,
- &&CASE_EEOP_ARRAYEXPR,
- &&CASE_EEOP_ARRAYCOERCE,
- &&CASE_EEOP_ROW,
- &&CASE_EEOP_ROWCOMPARE_STEP,
- &&CASE_EEOP_ROWCOMPARE_FINAL,
- &&CASE_EEOP_MINMAX,
- &&CASE_EEOP_FIELDSELECT,
- &&CASE_EEOP_FIELDSTORE_DEFORM,
- &&CASE_EEOP_FIELDSTORE_FORM,
- &&CASE_EEOP_SBSREF_SUBSCRIPTS,
- &&CASE_EEOP_SBSREF_OLD,
- &&CASE_EEOP_SBSREF_ASSIGN,
- &&CASE_EEOP_SBSREF_FETCH,
- &&CASE_EEOP_DOMAIN_TESTVAL,
- &&CASE_EEOP_DOMAIN_NOTNULL,
- &&CASE_EEOP_DOMAIN_CHECK,
- &&CASE_EEOP_CONVERT_R
Re: Improve readability by using designated initializers when possible
On Wed, 06 Mar 2024 at 01:53, Jelte Fennema-Nio wrote:
> On Tue, 5 Mar 2024 at 15:30, Japin Li wrote:
>> There is a warning if remove it, so I keep it.
>>
>> /home/japin/Codes/postgres/build/../src/backend/executor/execExprInterp.c:118:33:
>> warning: label ‘CASE_EEOP_LAST’ defined but not used [-Wunused-label]
>> 118 | #define EEO_CASE(name) CASE_##name:
>> | ^
>> /home/japin/Codes/postgres/build/../src/backend/executor/execExprInterp.c:1845:17:
>> note: in expansion of macro ‘EEO_CASE’
>> 1845 | EEO_CASE(EEOP_LAST)
>> | ^~~~
>
> I think if you remove the EEO_CASE(EEOP_LAST) block the warning should
> go away. That block is clearly marked as unreachable, so it doesn't
> really serve a purpose.
Thanks! Fixed as you suggested. Please see v3 patch.
>From ff4d9ce75cfd35a1865bbfb9cd5664a7806d92be Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Tue, 5 Mar 2024 21:32:46 +0800
Subject: [PATCH v3 1/1] Use C99-designated initializer syntax for
dispatch_table array
---
src/backend/executor/execExprInterp.c | 203 --
src/include/executor/execExpr.h | 2 +-
2 files changed, 97 insertions(+), 108 deletions(-)
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 7c1f51e2e0..df9fe79058 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -400,110 +400,106 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
TupleTableSlot *outerslot;
TupleTableSlot *scanslot;
- /*
- * This array has to be in the same order as enum ExprEvalOp.
- */
#if defined(EEO_USE_COMPUTED_GOTO)
static const void *const dispatch_table[] = {
- &&CASE_EEOP_DONE,
- &&CASE_EEOP_INNER_FETCHSOME,
- &&CASE_EEOP_OUTER_FETCHSOME,
- &&CASE_EEOP_SCAN_FETCHSOME,
- &&CASE_EEOP_INNER_VAR,
- &&CASE_EEOP_OUTER_VAR,
- &&CASE_EEOP_SCAN_VAR,
- &&CASE_EEOP_INNER_SYSVAR,
- &&CASE_EEOP_OUTER_SYSVAR,
- &&CASE_EEOP_SCAN_SYSVAR,
- &&CASE_EEOP_WHOLEROW,
- &&CASE_EEOP_ASSIGN_INNER_VAR,
- &&CASE_EEOP_ASSIGN_OUTER_VAR,
- &&CASE_EEOP_ASSIGN_SCAN_VAR,
- &&CASE_EEOP_ASSIGN_TMP,
- &&CASE_EEOP_ASSIGN_TMP_MAKE_RO,
- &&CASE_EEOP_CONST,
- &&CASE_EEOP_FUNCEXPR,
- &&CASE_EEOP_FUNCEXPR_STRICT,
- &&CASE_EEOP_FUNCEXPR_FUSAGE,
- &&CASE_EEOP_FUNCEXPR_STRICT_FUSAGE,
- &&CASE_EEOP_BOOL_AND_STEP_FIRST,
- &&CASE_EEOP_BOOL_AND_STEP,
- &&CASE_EEOP_BOOL_AND_STEP_LAST,
- &&CASE_EEOP_BOOL_OR_STEP_FIRST,
- &&CASE_EEOP_BOOL_OR_STEP,
- &&CASE_EEOP_BOOL_OR_STEP_LAST,
- &&CASE_EEOP_BOOL_NOT_STEP,
- &&CASE_EEOP_QUAL,
- &&CASE_EEOP_JUMP,
- &&CASE_EEOP_JUMP_IF_NULL,
- &&CASE_EEOP_JUMP_IF_NOT_NULL,
- &&CASE_EEOP_JUMP_IF_NOT_TRUE,
- &&CASE_EEOP_NULLTEST_ISNULL,
- &&CASE_EEOP_NULLTEST_ISNOTNULL,
- &&CASE_EEOP_NULLTEST_ROWISNULL,
- &&CASE_EEOP_NULLTEST_ROWISNOTNULL,
- &&CASE_EEOP_BOOLTEST_IS_TRUE,
- &&CASE_EEOP_BOOLTEST_IS_NOT_TRUE,
- &&CASE_EEOP_BOOLTEST_IS_FALSE,
- &&CASE_EEOP_BOOLTEST_IS_NOT_FALSE,
- &&CASE_EEOP_PARAM_EXEC,
- &&CASE_EEOP_PARAM_EXTERN,
- &&CASE_EEOP_PARAM_CALLBACK,
- &&CASE_EEOP_CASE_TESTVAL,
- &&CASE_EEOP_MAKE_READONLY,
- &&CASE_EEOP_IOCOERCE,
- &&CASE_EEOP_IOCOERCE_SAFE,
- &&CASE_EEOP_DISTINCT,
- &&CASE_EEOP_NOT_DISTINCT,
- &&CASE_EEOP_NULLIF,
- &&CASE_EEOP_SQLVALUEFUNCTION,
- &&CASE_EEOP_CURRENTOFEXPR,
- &&CASE_EEOP_NEXTVALUEEXPR,
- &&CASE_EEOP_ARRAYEXPR,
- &&CASE_EEOP_ARRAYCOERCE,
- &&CASE_EEOP_ROW,
- &&CASE_EEOP_ROWCOMPARE_STEP,
- &&CASE_EEOP_ROWCOMPARE_FINAL,
- &&CASE_EEOP_MINMAX,
- &&CASE_EEOP_FIELDSELECT,
- &&CASE_EEOP_FIELDSTORE_DEFORM,
- &&CASE_EEOP_FIELDSTORE_FORM,
- &&CASE_EEOP_SBSREF_SUBSCRIPTS,
- &&CASE_EEOP_SBSREF_OLD,
- &&CASE_EEOP_SBSREF_ASSIGN,
- &&CASE_EEOP_SBSREF_FETCH,
- &&CASE_EEOP_DOMAIN_TESTVAL,
- &&CASE_EEOP_DOMAIN_NOTNULL,
- &&CASE_EEOP_DOMAIN_CHECK,
- &&CASE_EEOP_CONVERT_ROWTYPE,
- &&CASE_EEOP_SCALARARRAYOP,
- &&CASE_EEOP_HASHED_SCALARARRAYOP,
- &&CASE_EEOP_XMLEXPR,
- &&CASE_EEOP_JSON_CONSTRUCTOR,
- &&CASE_EEOP_IS_JSON,
- &&CASE_EEOP_AGGREF,
- &&CASE_EEOP_GROUPING_FUNC,
- &&CASE_EEOP_WINDOW_FUNC,
- &&CASE_EEOP_SUBPLAN,
- &&CASE_EEOP_AGG_STRICT_DESERIALIZE,
- &&CASE_EEOP_AGG_DESERIALIZE,
Re: Improve readability by using designated initializers when possible
On Fri, 08 Mar 2024 at 13:21, Michael Paquier wrote: > On Wed, Mar 06, 2024 at 08:24:09AM +0800, Japin Li wrote: >> On Wed, 06 Mar 2024 at 01:53, Jelte Fennema-Nio wrote: >>> I think if you remove the EEO_CASE(EEOP_LAST) block the warning should >>> go away. That block is clearly marked as unreachable, so it doesn't >>> really serve a purpose. >> >> Thanks! Fixed as you suggested. Please see v3 patch. > > Hmm. I am not sure if this one is a good idea. Sorry for the late reply! > This makes the code a > bit more complicated to grasp under EEO_USE_COMPUTED_GOTO with the > reverse dispatch table, to say the least. I'm not get your mind. Could you explain in more detail?
Typo macro name on FreeBSD?
Hi, hackers Recently, I compile PostgreSQL on FreeBSD, I find commit a2a8acd152 introduecs __freebsd__ macro, however, I cannot find this macro on FreeBSD 13. There only has __FreeBSD__ macro. Is this a typo? root@freebsd:~ # uname -a FreeBSD freebsd 13.1-RELEASE-p3 FreeBSD 13.1-RELEASE-p3 GENERIC amd64 root@freebsd:~ # echo | gcc10 -dM -E - | grep -i 'freebsd' #define __FreeBSD__ 13 -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Typo macro name on FreeBSD?
On Fri, 16 Dec 2022 at 12:25, Thomas Munro wrote: > On Fri, Dec 16, 2022 at 4:44 PM Japin Li wrote: >> Recently, I compile PostgreSQL on FreeBSD, I find commit a2a8acd152 >> introduecs >> __freebsd__ macro, however, I cannot find this macro on FreeBSD 13. There >> only >> has __FreeBSD__ macro. Is this a typo? > > Yeah, that seems to be my fault. Will fix. Thanks! Thanks! -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
On Mon, 19 Dec 2022 at 22:40, Maxim Orlov wrote:
> Hi!
>
> As a result of discussion in the thread [0], Robert Haas proposed to focus
> on making SLRU 64 bit, as a first step towards 64 bit XIDs.
> Here is the patch set.
>
> In overall, code of this patch set is based on the existing code from [0]
> and may be simplified, due to the fact, that SLRU_PAGES_PER_SEGMENT is not
> meant to be changed now.
> But I decided to leave it that way. At least for now.
>
> As always, reviews and opinions are very welcome.
>
For v51-0003. We can use GetClogDirName instead of GET_MAJOR_VERSION in
copy_subdir_files().
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 1c49c63444..3934978b97 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -857,10 +857,7 @@ copy_xact_xlog_xid(void)
pfree(new_path);
}
else
- copy_subdir_files(GET_MAJOR_VERSION(old_cluster.major_version)
<= 906 ?
- "pg_clog" : "pg_xact",
-
GET_MAJOR_VERSION(new_cluster.major_version) <= 906 ?
- "pg_clog" : "pg_xact");
+ copy_subdir_files(GetClogDirName(old_cluster),
GetClogDirName(new_cluster));
prep_status("Setting oldest XID for new cluster");
exec_prog(UTILITY_LOG_FILE, NULL, true, true,
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
