Re: Support event trigger for logoff
On Sat, 20 Apr 2024 at 01:36, Tom Lane wrote: > Japin Li writes: >> I see [1] has already implemented on login event trigger, why not implement >> the logoff event trigger? > > What happens if the session crashes, or otherwise fails unexpectedly? > I am currently unsure how to handle such situations, but I believe it is not only for logoff events. >> Here is a problem with the regression test when using \c to create a new >> session, because it might be running concurrently, which may lead to the >> checking being unstable. > >> Any thoughts? > > Let's not go there at all. I don't think there's enough field > demand to justify dealing with this concept. > Thanks for your point out this for me. -- Regards, Japin Li
Support event trigger for logoff
Hi, hackers I see [1] has already implemented on login event trigger, why not implement the logoff event trigger? My friend Song Jinzhou and I try to implement the logoff event trigger, so attach it. Here is a problem with the regression test when using \c to create a new session, because it might be running concurrently, which may lead to the checking being unstable. Any thoughts? [1] https://www.postgresql.org/message-id/0d46d29f-4558-3af9-9c85-7774e14a7709%40postgrespro.ru -- Regards, Japin Li >From ef7c6aa408d0a6a97d7b0d2e093b71e279b1b0dc Mon Sep 17 00:00:00 2001 From: Japin Li Date: Mon, 15 Apr 2024 09:11:41 +0800 Subject: [PATCH v1 1/1] Add support event triggers for logoff --- doc/src/sgml/catalogs.sgml| 13 ++ doc/src/sgml/ecpg.sgml| 2 + doc/src/sgml/event-trigger.sgml | 5 + src/backend/commands/dbcommands.c | 17 +- src/backend/commands/event_trigger.c | 181 +- src/backend/tcop/postgres.c | 8 + src/backend/utils/cache/evtcache.c| 2 + src/backend/utils/init/globals.c | 1 + src/backend/utils/init/postinit.c | 1 + src/bin/psql/tab-complete.c | 2 +- src/include/catalog/pg_database.dat | 2 +- src/include/catalog/pg_database.h | 3 + src/include/commands/event_trigger.h | 1 + src/include/miscadmin.h | 1 + src/include/tcop/cmdtaglist.h | 1 + src/include/utils/evtcache.h | 1 + .../regress/expected/event_trigger_logoff.out | 49 + src/test/regress/parallel_schedule| 2 + src/test/regress/sql/event_trigger_logoff.sql | 26 +++ 19 files changed, 309 insertions(+), 9 deletions(-) create mode 100644 src/test/regress/expected/event_trigger_logoff.out create mode 100644 src/test/regress/sql/event_trigger_logoff.sql diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 2907079e2a..621fbfde98 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -3076,6 +3076,19 @@ SCRAM-SHA-256$iteration count: + + + dathaslogoffevt bool + + +Indicates that there are logoff event triggers defined for this database. +This flag is used to avoid extra lookups on the +pg_event_trigger table during each backend +startup. This flag is used internally by PostgreSQL +and should not be manually altered or read for monitoring purposes. + + + datconnlimit int4 diff --git a/doc/src/sgml/ecpg.sgml b/doc/src/sgml/ecpg.sgml index e7a53f3c9d..d223843157 100644 --- a/doc/src/sgml/ecpg.sgml +++ b/doc/src/sgml/ecpg.sgml @@ -4765,6 +4765,7 @@ encoding = 0 (type: 5) datistemplate = t (type: 1) datallowconn = t (type: 1) dathasloginevt = f (type: 1) +dathaslogoffevt = f (type: 1) datconnlimit = -1 (type: 5) datfrozenxid = 379 (type: 1) dattablespace = 1663 (type: 1) @@ -4790,6 +4791,7 @@ encoding = 0 (type: 5) datistemplate = f (type: 1) datallowconn = t (type: 1) dathasloginevt = f (type: 1) +dathaslogoffevt = f (type: 1) datconnlimit = -1 (type: 5) datfrozenxid = 379 (type: 1) dattablespace = 1663 (type: 1) diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml index 8e009cca05..ed996a456e 100644 --- a/doc/src/sgml/event-trigger.sgml +++ b/doc/src/sgml/event-trigger.sgml @@ -29,6 +29,7 @@ occurs in the database in which it is defined. Currently, the only supported events are login, + logoff, ddl_command_start, ddl_command_end, table_rewrite @@ -54,6 +55,10 @@ the in-progress login trigger. + + The logoff event occurs when a user logs off the system. + + The ddl_command_start event occurs just before the execution of a CREATE, ALTER, DROP, diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index 8229dfa1f2..a6784a0ea6 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -116,7 +116,7 @@ static void movedb_failure_callback(int code, Datum arg); static bool get_db_info(const char *name, LOCKMODE lockmode, Oid *dbIdP, Oid *ownerIdP, int *encodingP, bool *dbIsTemplateP, bool *dbAllowConnP, bool *dbHasLoginEvtP, - TransactionId *dbFrozenXidP, MultiXactId *dbMinMultiP, + bool *dbHasLogoffEvtP, TransactionId *dbFrozenXidP, MultiXactId *dbMinMultiP, Oid *dbTablespace, char **dbCollate, char **dbCtype, char **dbLocale, char **dbIcurules, char *dbLocProvider, @@ -680,6 +680,7 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt) char *src_collversion = NULL; bool src_istemplate; bool src_hasloginevt = false; + bool src_haslogoffevt = false; bool src_allowconn; TransactionId src_frozenxid = InvalidTransactionId; MultiXac
Re: Cannot find a working 64-bit integer type on Illumos
On Fri, 19 Apr 2024 at 05:22, Thomas Munro wrote:
> On Thu, Apr 18, 2024 at 6:09 PM Japin Li wrote:
>> /home/japin/postgres/build/../src/include/lib/simplehash.h:1138:9: error:
>> format '%llu' expects argument of type 'long long unsigned int', but
>> argument 4 has type 'uint64' {aka 'long unsigned int'} [-Werror=format=]
>
> It seems my v1 patch's configure probe for INT64_FORMAT was broken.
> In the v2 patch I tried not doing that probe at all, and instead
> inviting into our world (that's the standardised way to
> produce format strings, which has the slight complication that we are
> intercepting printf calls...). I suspect that'll work better for you.
Yeah, the v2 patch fixed this problem.
--
Regards,
Japin Li
Re: Cannot find a working 64-bit integer type on Illumos
On Thu, 18 Apr 2024 at 08:31, Thomas Munro wrote:
> On Sat, Mar 23, 2024 at 3:23 PM Tom Lane wrote:
>> Thomas Munro writes:
>> > . o O ( int64_t, PRIdi64, etc were standardised a quarter of a century ago
>> > )
>>
>> Yeah. Now that we require C99 it's probably reasonable to assume
>> that those things exist. I wouldn't be in favor of ripping out our
>> existing notations like UINT64CONST, because the code churn would be
>> substantial and the gain minimal. But we could imagine reimplementing
>> that stuff atop and then getting rid of the configure-time
>> probes.
>
> I played around with this a bit, but am not quite there yet.
>
> printf() is a little tricky. The standard wants us to use
> 's PRId64 etc, but that might confuse our snprintf.c (in
> theory, probably not in practice). "ll" should have the right size on
> all systems, but gets warnings from the printf format string checker
> on systems where "l" is the right type. So I think we still need to
> probe for INT64_MODIFIER at configure-time. Here's one way, but I can
> see it's not working on Clang/Linux... perhaps instead of that dubious
> incantation I should try compiling some actual printfs and check for
> warnings/errors.
>
> I think INT64CONST should just point to standard INT64_C().
>
> For limits, why do we have this:
>
> - * stdint.h limits aren't guaranteed to have compatible types with our fixed
> - * width types. So just define our own.
>
> ? I mean, how could they not have compatible types?
>
> I noticed that configure.ac checks if int64 (no "_t") might be defined
> already by system header pollution, but meson.build doesn't. That's
> an inconsistency that should be fixed, but which way? Hmm, commit
> 15abc7788e6 said that was done for BeOS, which we de-supported. So
> maybe we should get rid of that?
Thanks for working on this! I test the patch and it now works on Illumos when
configure with -Werror option. However, there are some errors when compiling.
In file included from /home/japin/postgres/build/../src/include/c.h:834,
from
/home/japin/postgres/build/../src/include/postgres_fe.h:25,
from /home/japin/postgres/build/../src/common/config_info.c:20:
/home/japin/postgres/build/../src/common/config_info.c: In function
'get_configdata':
/home/japin/postgres/build/../src/common/config_info.c:198:11: error:
comparison of integer expressions of different signedness: 'int' and 'size_t'
{aka 'long unsigned int'} [-Werror=sign-compare]
198 | Assert(i == *configdata_len);
| ^~
/home/japin/postgres/build/../src/common/config_info.c:198:2: note: in
expansion of macro 'Assert'
198 | Assert(i == *configdata_len);
| ^~
In file included from /home/japin/postgres/build/../src/common/blkreftable.c:36:
/home/japin/postgres/build/../src/include/lib/simplehash.h: In function
'blockreftable_stat':
/home/japin/postgres/build/../src/include/lib/simplehash.h:1138:9: error:
format '%llu' expects argument of type 'long long unsigned int', but argument 4
has type 'uint64' {aka 'long unsigned int'} [-Werror=format=]
1138 | sh_log("size: " UINT64_FORMAT ", members: %u, filled: %f, total chain:
%u, max chain: %u, avg chain: %f, total_collisions: %u, max_collisions: %u,
avg_collisions: %f",
| ^~~~
1139 | tb->size, tb->members, fillfactor, total_chain_length,
max_chain_length, avg_chain_length,
|
||
|uint64 {aka long unsigned int}
/home/japin/postgres/build/../src/include/common/logging.h:125:46: note: in
definition of macro 'pg_log_info'
125 | pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__)
| ^~~
/home/japin/postgres/build/../src/include/lib/simplehash.h:1138:2: note: in
expansion of macro 'sh_log'
1138 | sh_log("size: " UINT64_FORMAT ", members: %u, filled: %f, total chain:
%u, max chain: %u, avg chain: %f, total_collisions: %u, max_collisions: %u,
avg_collisions: %f",
| ^~
In file included from
/home/japin/postgres/build/../src/include/access/xlogrecord.h:14,
from
/home/japin/postgres/build/../src/include/access/xlogreader.h:41,
from
/home/japin/postgres/build/../src/include/access/xlog_internal.h:23,
from
/home/japin/postgres/build/../src/common/controldata_utils.c:28:
/home/japin/postgres/build/../src/include/access/rmgr.h: In function
'RmgrIdIsCustom':
/home/japin/postgres/build/../src/include/access/rmgr.h:50:42: error:
comparison of integer expressions of different signedness: 'int' and 'unsigned
int' [-Werror=sign-compare]
50 | return rmid >= RM_MIN_CUSTOM_ID && rmid <= RM_MAX_CUSTOM_ID;
Re: Typo about the SetDatatabaseHasLoginEventTriggers?
On Wed, 17 Apr 2024 at 14:10, Michael Paquier wrote: > On Tue, Apr 16, 2024 at 03:31:49PM +0900, Michael Paquier wrote: >> Indeed, thanks! Will fix and double-check the surroundings. > > And fixed this one. Thanks for the pushing! -- Regards, Japin Li
Typo about the SetDatatabaseHasLoginEventTriggers?
Hi,
Upon reviewing the login event trigger, I noticed a potential typo about
the SetDatabaseHasLoginEventTriggers function name.
diff --git a/src/backend/commands/event_trigger.c
b/src/backend/commands/event_trigger.c
index 0d3214df9c..7a5ed6b985 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -111,7 +111,7 @@ static void validate_table_rewrite_tags(const char
*filtervar, List *taglist);
static void EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata);
static const char *stringify_grant_objtype(ObjectType objtype);
static const char *stringify_adefprivs_objtype(ObjectType objtype);
-static void SetDatatabaseHasLoginEventTriggers(void);
+static void SetDatabaseHasLoginEventTriggers(void);
/*
* Create an event trigger.
@@ -315,7 +315,7 @@ insert_event_trigger_tuple(const char *trigname, const char
*eventname, Oid evtO
* faster lookups in hot codepaths. Set the flag unless already True.
*/
if (strcmp(eventname, "login") == 0)
- SetDatatabaseHasLoginEventTriggers();
+ SetDatabaseHasLoginEventTriggers();
/* Depend on owner. */
recordDependencyOnOwner(EventTriggerRelationId, trigoid, evtOwner);
@@ -383,7 +383,7 @@ filter_list_to_array(List *filterlist)
* current database has on login event triggers.
*/
void
-SetDatatabaseHasLoginEventTriggers(void)
+SetDatabaseHasLoginEventTriggers(void)
{
/* Set dathasloginevt flag in pg_database */
Form_pg_database db;
@@ -453,7 +453,7 @@ AlterEventTrigger(AlterEventTrigStmt *stmt)
*/
if (namestrcmp(>evtevent, "login") == 0 &&
tgenabled != TRIGGER_DISABLED)
- SetDatatabaseHasLoginEventTriggers();
+ SetDatabaseHasLoginEventTriggers();
InvokeObjectPostAlterHook(EventTriggerRelationId,
trigoid, 0);
@@ -925,7 +925,7 @@ EventTriggerOnLogin(void)
/*
* There is no active login event trigger, but our
* pg_database.dathasloginevt is set. Try to unset this flag. We use
the
-* lock to prevent concurrent SetDatatabaseHasLoginEventTriggers(), but
we
+* lock to prevent concurrent SetDatabaseHasLoginEventTriggers(), but we
* don't want to hang the connection waiting on the lock. Thus, we are
* just trying to acquire the lock conditionally.
*/
--
Regards,
Japin Li
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
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 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
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: 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 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.
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: 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
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?
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[] = {
- &_EEOP_DONE,
- &_EEOP_INNER_FETCHSOME,
- &_EEOP_OUTER_FETCHSOME,
- &_EEOP_SCAN_FETCHSOME,
- &_EEOP_INNER_VAR,
- &_EEOP_OUTER_VAR,
- &_EEOP_SCAN_VAR,
- &_EEOP_INNER_SYSVAR,
- &_EEOP_OUTER_SYSVAR,
- &_EEOP_SCAN_SYSVAR,
- &_EEOP_WHOLEROW,
- &_EEOP_ASSIGN_INNER_VAR,
- &_EEOP_ASSIGN_OUTER_VAR,
- &_EEOP_ASSIGN_SCAN_VAR,
- &_EEOP_ASSIGN_TMP,
- &_EEOP_ASSIGN_TMP_MAKE_RO,
- &_EEOP_CONST,
- &_EEOP_FUNCEXPR,
- &_EEOP_FUNCEXPR_STRICT,
- &_EEOP_FUNCEXPR_FUSAGE,
- &_EEOP_FUNCEXPR_STRICT_FUSAGE,
- &_EEOP_BOOL_AND_STEP_FIRST,
- &_EEOP_BOOL_AND_STEP,
- &_EEOP_BOOL_AND_STEP_LAST,
- &_EEOP_BOOL_OR_STEP_FIRST,
- &_EEOP_BOOL_OR_STEP,
- &_EEOP_BOOL_OR_STEP_LAST,
- &_EEOP_BOOL_NOT_STEP,
- &_EEOP_QUAL,
- &_EEOP_JUMP,
- &_EEOP_JUMP_IF_NULL,
- &_EEOP_JUMP_IF_NOT_NULL,
- &_EEOP_JUMP_IF_NOT_TRUE,
- &_EEOP_NULLTEST_ISNULL,
- &_EEOP_NULLTEST_ISNOTNULL,
- &_EEOP_NULLTEST_ROWISNULL,
- &_EEOP_NULLTEST_ROWISNOTNULL,
- &_EEOP_BOOLTEST_IS_TRUE,
- &_EEOP_BOOLTEST_IS_NOT_TRUE,
- &_EEOP_BOOLTEST_IS_FALSE,
- &_EEOP_BOOLTEST_IS_NOT_FALSE,
- &_EEOP_PARAM_EXEC,
- &_EEOP_PARAM_EXTERN,
- &_EEOP_PARAM_CALLBACK,
- &_EEOP_CASE_TESTVAL,
- &_EEOP_MAKE_READONLY,
- &_EEOP_IOCOERCE,
- &_EEOP_IOCOERCE_SAFE,
- &_EEOP_DISTINCT,
- &_EEOP_NOT_DISTINCT,
- &_EEOP_NULLIF,
- &_EEOP_SQLVALUEFUNCTION,
- &_EEOP_CURRENTOFEXPR,
- &_EEOP_NEXTVALUEEXPR,
- &_EEOP_ARRAYEXPR,
- &_EEOP_ARRAYCOERCE,
- &_EEOP_ROW,
- &_EEOP_ROWCOMPARE_STEP,
- &_EEOP_ROWCOMPARE_FINAL,
- &_EEOP_MINMAX,
- &_EEOP_FIELDSELECT,
- &_EEOP_FIELDSTORE_DEFORM,
- &_EEOP_FIELDSTORE_FORM,
- &_EEOP_SBSREF_SUBSCRIPTS,
- &_EEOP_SBSREF_OLD,
- &_EEOP_SBSREF_ASSIGN,
- &_EEOP_SBSREF_FETCH,
- &_EEOP_DOMAIN_TESTVAL,
- &_EEOP_DOMAIN_NOTNULL,
- &_EEOP_DOMAIN_CHECK,
- &_EEOP_CONVERT_ROWTYPE,
- &_EEOP_SCALARARRAYOP,
- &_EEOP_HASHED_SCALARARRAYOP,
- &_EEOP_XMLEXPR,
- &_EEOP_JSON_CONSTRUCTOR,
- &_EEOP_IS_JSON,
- &_EEOP_AGGREF,
- &_EEOP_GROUPING_FUNC,
- &_EEOP_WINDOW_FUNC,
- &_EEOP_SUBPLAN,
- &_EEOP_AGG_STRICT_DESERIALIZE,
- &_EEOP_AGG_DESERIALIZE,
- &_EEOP_AGG_STRICT_INPUT_CHECK_ARGS,
- &_EEOP_AGG_STRICT_INPUT_CHECK_NULLS,
- &_EEOP_AGG_PLAIN_PERGROUP_NULLCHECK,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_BYREF,
- &_EEOP_AGG_PRESORTED_DISTINCT_SINGLE,
- &_EEOP_AGG_PRESORTED_DISTINCT_MULTI,
- &_EEOP_AGG_ORDERED_TRANS_DATUM,
- &_EEOP_AGG_ORDERED_TRANS_TUPLE,
- &_EEOP_LAST
+ [EEOP_DONE] = &_EEOP_DONE,
+ [EEOP_INNER_FETCHSOME] = &_EEOP_INNER_FETCHSOME,
+ [EEOP_OUTER_FETCHSOME] = &_EEOP_OUTER_FETCHSOME,
+ [EEOP_SCAN_FETCHSOME] =
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] = &_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[] = {
- &_EEOP_DONE,
- &_EEOP_INNER_FETCHSOME,
- &_EEOP_OUTER_FETCHSOME,
- &_EEOP_SCAN_FETCHSOME,
- &_EEOP_INNER_VAR,
- &_EEOP_OUTER_VAR,
- &_EEOP_SCAN_VAR,
- &_EEOP_INNER_SYSVAR,
- &_EEOP_OUTER_SYSVAR,
- &_EEOP_SCAN_SYSVAR,
- &_EEOP_WHOLEROW,
- &_EEOP_ASSIGN_INNER_VAR,
- &_EEOP_ASSIGN_OUTER_VAR,
- &_EEOP_ASSIGN_SCAN_VAR,
- &_EEOP_ASSIGN_TMP,
- &_EEOP_ASSIGN_TMP_MAKE_RO,
- &_EEOP_CONST,
- &_EEOP_FUNCEXPR,
- &_EEOP_FUNCEXPR_STRICT,
- &_EEOP_FUNCEXPR_FUSAGE,
- &_EEOP_FUNCEXPR_STRICT_FUSAGE,
- &_EEOP_BOOL_AND_STEP_FIRST,
- &_EEOP_BOOL_AND_STEP,
- &_EEOP_BOOL_AND_STEP_LAST,
- &_EEOP_BOOL_OR_STEP_FIRST,
- &_EEOP_BOOL_OR_STEP,
- &_EEOP_BOOL_OR_STEP_LAST,
- &_EEOP_BOOL_NOT_STEP,
- &_EEOP_QUAL,
- &_EEOP_JUMP,
- &_EEOP_JUMP_IF_NULL,
- &_EEOP_JUMP_IF_NOT_NULL,
- &_EEOP_JUMP_IF_NOT_TRUE,
- &_EEOP_NULLTEST_ISNULL,
- &_EEOP_NULLTEST_ISNOTNULL,
- &_EEOP_NULLTEST_ROWISNULL,
- &_EEOP_NULLTEST_ROWISNOTNULL,
- &_EEOP_BOOLTEST_IS_TRUE,
- &_EEOP_BOOLTEST_IS_NOT_TRUE,
- &_EEOP_BOOLTEST_IS_FALSE,
- &_EEOP_BOOLTEST_IS_NOT_FALSE,
- &_EEOP_PARAM_EXEC,
- &_EEOP_PARAM_EXTERN,
- &_EEOP_PARAM_CALLBACK,
- &_EEOP_CASE_TESTVAL,
- &_EEOP_MAKE_READONLY,
- &_EEOP_IOCOERCE,
- &_EEOP_IOCOERCE_SAFE,
- &_EEOP_DISTINCT,
- &_EEOP_NOT_DISTINCT,
- &_EEOP_NULLIF,
- &_EEOP_SQLVALUEFUNCTION,
- &_EEOP_CURRENTOFEXPR,
- &_EEOP_NEXTVALUEEXPR,
- &_EEOP_ARRAYEXPR,
- &_EEOP_ARRAYCOERCE,
- &_EEOP_ROW,
- &_EEOP_ROWCOMPARE_STEP,
- &_EEOP_ROWCOMPARE_FINAL,
- &_EEOP_MINMAX,
- &_EEOP_FIELDSELECT,
- &_EEOP_FIELDSTORE_DEFORM,
- &_EEOP_FIELDSTORE_FORM,
- &_EEOP_SBSREF_SUBSCRIPTS,
- &_EEOP_SBSREF_OLD,
- &_EEOP_SBSREF_ASSIGN,
- &_EEOP_SBSREF_FETCH,
- &_EEOP_DOMAIN_TESTVAL,
- &_EEOP_DOMAIN_NOTNULL,
- &_EEOP_DOMAIN_CHECK,
- &_EEOP_CONVERT_ROWTYPE,
- &_EEOP_SCALARARRAYOP,
- &_EEOP_HASHED_SCALARARRAYOP,
- &_EEOP_XMLEXPR,
- &_EEOP_JSON_CONSTRUCTOR,
- &_EEOP_IS_JSON,
- &_EEOP_AGGREF,
- &_EEOP_GROUPING_FUNC,
- &_EEOP_WINDOW_FUNC,
- &_EEOP_SUBPLAN,
- &_EEOP_AGG_STRICT_DESERIALIZE,
- &_EEOP_AGG_DESERIALIZE,
- &_EEOP_AGG_STRICT_INPUT_CHECK_ARGS,
- &_EEOP_AGG_STRICT_INPUT_CHECK_NULLS,
- &_EEOP_AGG_PLAIN_PERGROUP_NULLCHECK,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYREF,
- &_EE
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[] = {
- &_EEOP_DONE,
- &_EEOP_INNER_FETCHSOME,
- &_EEOP_OUTER_FETCHSOME,
- &_EEOP_SCAN_FETCHSOME,
- &_EEOP_INNER_VAR,
- &_EEOP_OUTER_VAR,
- &_EEOP_SCAN_VAR,
- &_EEOP_INNER_SYSVAR,
- &_EEOP_OUTER_SYSVAR,
- &_EEOP_SCAN_SYSVAR,
- &_EEOP_WHOLEROW,
- &_EEOP_ASSIGN_INNER_VAR,
- &_EEOP_ASSIGN_OUTER_VAR,
- &_EEOP_ASSIGN_SCAN_VAR,
- &_EEOP_ASSIGN_TMP,
- &_EEOP_ASSIGN_TMP_MAKE_RO,
- &_EEOP_CONST,
- &_EEOP_FUNCEXPR,
- &_EEOP_FUNCEXPR_STRICT,
- &_EEOP_FUNCEXPR_FUSAGE,
- &_EEOP_FUNCEXPR_STRICT_FUSAGE,
- &_EEOP_BOOL_AND_STEP_FIRST,
- &_EEOP_BOOL_AND_STEP,
- &_EEOP_BOOL_AND_STEP_LAST,
- &_EEOP_BOOL_OR_STEP_FIRST,
- &_EEOP_BOOL_OR_STEP,
- &_EEOP_BOOL_OR_STEP_LAST,
- &_EEOP_BOOL_NOT_STEP,
- &_EEOP_QUAL,
- &_EEOP_JUMP,
- &_EEOP_JUMP_IF_NULL,
- &_EEOP_JUMP_IF_NOT_NULL,
- &_EEOP_JUMP_IF_NOT_TRUE,
- &_EEOP_NULLTEST_ISNULL,
- &_EEOP_NULLTEST_ISNOTNULL,
- &_EEOP_NULLTEST_ROWISNULL,
- &_EEOP_NULLTEST_ROWISNOTNULL,
- &_EEOP_BOOLTEST_IS_TRUE,
- &_EEOP_BOOLTEST_IS_NOT_TRUE,
- &_EEOP_BOOLTEST_IS_FALSE,
- &_EEOP_BOOLTEST_IS_NOT_FALSE,
- &_EEOP_PARAM_EXEC,
- &_EEOP_PARAM_EXTERN,
- &_EEOP_PARAM_CALLBACK,
- &_EEOP_CASE_TESTVAL,
- &_EEOP_MAKE_READONLY,
- &_EEOP_IOCOERCE,
- &_EEOP_IOCOERCE_SAFE,
- &_EEOP_DISTINCT,
- &_EEOP_NOT_DISTINCT,
- &_EEOP_NULLIF,
- &_EEOP_SQLVALUEFUNCTION,
- &_EEOP_CURRENTOFEXPR,
- &_EEOP_NEXTVALUEEXPR,
- &_EEOP_ARRAYEXPR,
- &_EEOP_ARRAYCOERCE,
- &_EEOP_ROW,
- &_EEOP_ROWCOMPARE_STEP,
- &_EEOP_ROWCOMPARE_FINAL,
- &_EEOP_MINMAX,
- &_EEOP_FIELDSELECT,
- &_EEOP_FIELDSTORE_DEFORM,
- &_EEOP_FIELDSTORE_FORM,
- &_EEOP_SBSREF_SUBSCRIPTS,
- &_EEOP_SBSREF_OLD,
- &_EEOP_SBSREF_ASSIGN,
- &_EEOP_SBSREF_FETCH,
- &_EEOP_DOMAIN_TESTVAL,
- &_EEOP_DOMAIN_NOTNULL,
- &_EEOP_DOMAIN_CHECK,
- &_EEOP_CONVERT_ROWTYPE,
- &_EEOP_SCALARARRAYOP,
- &_EEOP_HASHED_SCALARARRAYOP,
- &_EEOP_XMLEXPR,
- &_EEOP_JSON_CONSTRUCTOR,
- &_EEOP_IS_JSON,
- &_EEOP_AGGREF,
- &_EEOP_GROUPING_FUNC,
- &_EEOP_WINDOW_FUNC,
- &_EEOP_SUBPLAN,
- &_EEOP_AGG_STRICT_DESERIALIZE,
- &_EEOP_AGG_DESERIALIZE,
- &_EEOP_AGG_STRICT_INPUT_CHECK_ARGS,
- &_EEOP_AGG_STRICT_INPUT_CHECK_NULLS,
- &_EEOP_AGG_PLAIN_PERGROUP_NULLCHECK,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_BYREF,
- &_EEOP_AGG_PRESORTED_DISTINCT_SINGLE,
- &_EEOP_AGG_PRESORTED_DISTINCT_MULTI,
- &_EEOP_AGG_ORDERED_TRANS_DATUM,
- &_EEOP_AGG_ORDERED_TRANS_TUPLE,
- &_EEOP_LAST
+ [EEOP_DONE] = &_EEOP_DONE,
+ [EEOP_INNER_FETCHSOME] = &_EEOP_INNER_FETCHSOME,
+ [EEOP_OUTER_FETCHSOME] = &_EEOP_OUTER_FETCHSOME,
+ [EEOP_SCAN_FETCHSOME] = &_EEOP_SCAN_FETCHSOME,
+ [EEOP_INNER_VAR] = &_EEOP_INNER_VAR,
+ [EEOP_OUTER_VAR] = &_EEOP_OUTER_VAR,
+ [EEOP_SCAN_VAR] = &_EEOP_SCAN_VAR,
+ [EEOP_INNER_SYSVAR] = &_EEOP_INNER_SYSVAR,
+ [EEOP_OUTER_SYSVAR] = &
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 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 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: 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: 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: 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: 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: Thoughts about NUM_BUFFER_PARTITIONS
> On Feb 10, 2024, at 20:15, Tomas Vondra wrote: > > On 2/8/24 14:27, wenhui qiu wrote: >> Hi Heikki Linnakangas >>I think the larger shared buffer higher the probability of multiple >> backend processes accessing the same bucket slot BufMappingLock >> simultaneously, （ InitBufTable(NBuffers + NUM_BUFFER_PARTITIONS); When I >> have free time, I want to do this test. I have seen some tests, but the >> result report is in Chinese >> > > I think Heikki is right this is unrelated to the amount of RAM. The > partitions are meant to reduce the number of lock collisions when > multiple processes try to map a buffer concurrently. But the machines > got much larger in this regard too - in 2006 the common CPUs had maybe > 2-4 cores, now it's common to have CPUs with ~100 cores, and systems > with multiple of them. OTOH the time spent holing the partition lock > should be pretty low, IIRC we pretty much just pin the buffer before > releasing it, and the backend should do plenty other expensive stuff. So > who knows how many backends end up doing the locking at the same time. > > OTOH, with 128 partitions it takes just 14 backends to have 50% chance > of a conflict, so with enough cores ... But how many partitions would be > enough? With 1024 partitions it still takes only 38 backends to get 50% > chance of a collision. Better, but considering we now have hundreds of > cores, not sure if sufficient. > > (Obviously, we probably want much lower probability of a collision, I > only used 50% to illustrate the changes). > 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?
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.
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: 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.
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
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.
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.
Re: Transaction timeout
On Wed, 03 Jan 2024 at 20:04, Andrey M. Borodin wrote: >> On 3 Jan 2024, at 16:46, Andrey M. Borodin wrote: >> >> I do not understand why, but mailing list did not pick patches that I sent. >> I'll retry. > > > Sorry for the noise. Seems like Apple updated something in Mail.App couple of > days ago and it started to use strange "Apple-Mail" stuff by default. > I see patches were attached, but were not recognized by mailing list archives > and CFbot. > Now I've flipped everything to "plain text by default" everywhere. Hope that > helps. > Thanks for updating the patch, I find the test on Debian with mason failed [1]. Does the timeout is too short for testing? I see the timeouts for lock_timeout and statement_timeout is more bigger than transaction_timeout. [1] https://api.cirrus-ci.com/v1/artifact/task/5490718928535552/testrun/build-32/testrun/isolation/isolation/regression.diffs
Re: Tab complete for CREATE SUBSCRIPTION ... CONECTION does not work
On Tue, 26 Dec 2023 at 18:10, Shubham Khanna wrote: > On Tue, Dec 26, 2023 at 3:02 PM Japin Li wrote: >> >> >> Hi hacker, >> >> As $subject detailed, the tab-complete cannot work such as: >> >>CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres port=6543' \t >> >> It seems that the get_previous_words() could not parse the single quote. >> >> OTOH, it works for CREATE SUBSCRIPTION sub CONNECTION xx \t, should we fix >> it? >> > I reviewed the Patch and it looks fine to me. > Thanks for the review. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Erroneous -Werror=missing-braces on old GCC
On Mon, 25 Dec 2023 at 10:42, Richard Guo wrote:
> I came across the 'missing braces' warning again when building master
> (0a93f803f4) on old GCC (4.8.5).
>
> blkreftable.c: In function ‘BlockRefTableSetLimitBlock’:
> blkreftable.c:268:2: warning: missing braces around initializer
> [-Wmissing-braces]
> BlockRefTableKey key = {0}; /* make sure any padding is zero */
> ^
>
I doubt if `key = {0}` equals `key = {{0}}`, since the second only
initialize the first field in `key`, it may depend on compiler to
initialize other fields (include padding).
> This has popped up a few times in the past, and it seems to be GCC bug
> 53119. We previously used the {{...}} approach to suppress it. Should
> we do the same here, like attached?
>
> FWIW, in the same file we initialize BlockRefTableSerializedEntry
> variables also with {{0}}.
>
> Thanks
> Richard
--
Regrads,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Sun, 24 Dec 2023 at 01:14, Andrey M. Borodin wrote: >> On 22 Dec 2023, at 10:39, Japin Li wrote: >> >> >> I try to split the test for transaction timeout, and all passed on my CI [1]. > > > I like the refactoring you did in timeout.spec. I thought it is impossible, > because permutations would try to reinitialize FATALed sessions. But, > obviously, tests work the way you refactored it. > However I don't think ignoring test failures on Windows without understanding > root cause is a good idea. Yeah. > Let's get back to v13 version of tests, understand why it failed, apply your > test refactorings afterwards. BTW are you sure that v14 refactorings are > functional equivalent of v13 tests? > I think it is equivalent. Maybe I missing something. Please let me known if they are not equivalent. > To go with this plan I attach slightly modified version of v13 tests in v16 > patchset. The only change is timing in "sleep_there" step. I suspect that > failure was induced by more coarse timer granularity on Windows. Tests were > giving only 9 milliseconds for a timeout to entirely wipe away backend from > pg_stat_activity. This saves testing time, but might induce false positive > test flaps. So I've raised wait times to 100ms. This seems too much, but I do > not have other ideas how to ensure tests stability. Maybe 50ms would be > enough, I do not know. Isolation runs ~50 seconds now. I'm tempted to say > that 200ms for timeouts worth it. > So this is caused by Windows timer granularity? > As to 2nd step "Try to enable transaction_timeout during transaction", I > think this makes sense. But if we are doing so, shouldn't we also allow to > enable idle_in_transaction timeout in a same manner? I think the current idle_in_transaction_session_timeout work correctly. > Currently we only allow to disable other timeouts... Also, if we are already > in transaction, shouldn't we also subtract current transaction span from > timeout? Agreed. > I think making this functionality as another step of the patchset was a good > idea. > -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Fixing pgbench init overflow
On Sat, 23 Dec 2023 at 15:22, Tatsuo Ishii wrote:
>
>
>
>>
>> On Sat, 23 Dec 2023 at 07:18, Chen Hao Hsu wrote:
>>> Hello,
>>>
>>> pgbench mixes int and int64 to initialize the tables.
>>> When a large enough scale factor is passed, initPopulateTable
>>> overflows leading to it never completing, ie.
>>>
>>> 214740 of 22 tuples (97%) of
>>> pgbench_accounts done (elapsed 4038.83 s, remaining 98.93 s)
>>> -214740 of 22 tuples (-97%) of
>>> pgbench_accounts done (elapsed 4038.97 s, remaining -8176.86 s)
>>>
>>>
>>> Attached is a patch that fixes this, pgbench -i -s 22000 works now.
>>
>> I think only the following line can fix this.
>>
>> +int64 k;
>>
>> Do not need to modify the type of `n`, right?
>
> You are right. n represents the return value of pg_snprintf, which is
> the byte length of the formatted data, which is int, not int64.
>
Thanks for you confirmation! Please consider the v2 patch to review.
--
Regrads,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
>From cee64786a2353cbf77d67db416ba0596a461690b Mon Sep 17 00:00:00 2001
From: John Hsu
Date: Fri, 22 Dec 2023 22:38:15 +
Subject: [PATCH v2 1/1] Fix pgbench init overflow when total number of rows is
greater than int32
Previously when the number of rows exceeds int32, it would
result in overflow and pgbench never finishing. This
change moves towards int64 to align with the comparison.
---
src/bin/pgbench/pgbench.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 2e1650d0ad..5b6549c89d 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4908,7 +4908,7 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
initRowMethod init_row)
{
int n;
- int k;
+ int64 k;
int chars = 0;
PGresult *res;
PQExpBufferData sql;
base-commit: 3e2e0d5ad7fcb89d18a71cbfc885ef184e1b6f2e
--
2.41.0
Re: Transaction timeout
> 在 2023年12月23日，11:35，Junwang Zhao 写道： > > On Sat, Dec 23, 2023 at 11:17 AM Japin Li wrote: >> >> a >>> On Sat, 23 Dec 2023 at 10:40, Japin Li wrote: >>> On Sat, 23 Dec 2023 at 08:32, Japin Li wrote: >>>> On Fri, 22 Dec 2023 at 23:30, Junwang Zhao wrote: >>>>> On Fri, Dec 22, 2023 at 10:44 PM Japin Li wrote: >>>>>> >>>>>> >>>>>> On Fri, 22 Dec 2023 at 22:37, Junwang Zhao wrote: >>>>>>> On Fri, Dec 22, 2023 at 10:25 PM Japin Li wrote: >>>>>>>> I try to set idle_in_transaction_session_timeout after begin >>>>>>>> transaction, >>>>>>>> it changes immediately, so I think transaction_timeout should also be >>>>>>>> take >>>>>>>> immediately. >>>>>>> >>>>>>> Ah, right, idle_in_transaction_session_timeout is set after the set >>>>>>> command finishes and before the backend send *ready for query* >>>>>>> to the client, so the value of the GUC is already set before >>>>>>> next command. >>>>>>> >>>>>> >>>>>> I mean, is it possible to set transaction_timeout before next comand? >>>>>> >>>>> Yeah, it's possible, set transaction_timeout in the when it first >>>>> goes into *idle in transaction* mode, see the attached files. >>>>> >>>> >>>> Thanks for updating the patch, LGTM. >>> >>> Sorry for the noise! >>> >>> Read the previous threads, I find why the author enable transaction_timeout >>> in start_xact_command(). >>> >>> The v15 patch cannot handle COMMIT AND CHAIN, see [1]. For example: >>> >>> SET transaction_timeout TO '2s'; BEGIN; SELECT 1, pg_sleep(1); COMMIT AND >>> CHAIN; SELECT 2, pg_sleep(1); COMMIT; >>> >>> The transaction_timeout do not reset when executing COMMIT AND CHAIN. >>> >>> [1] >>> https://www.postgresql.org/message-id/a906dea1-76a1-4f26-76c5-a7efad3ef5b8%40oss.nttdata.com >> >> Attach v16 to solve this. Any suggestions? > > I've checked this with *COMMIT AND CHAIN* and *ABORT AND CHAIN*, > both work as expected. Thanks for the update. > Thanks for your testing and reviewing!
Re: Fixing pgbench init overflow
On Sat, 23 Dec 2023 at 07:18, Chen Hao Hsu wrote: > Hello, > > pgbench mixes int and int64 to initialize the tables. > When a large enough scale factor is passed, initPopulateTable > overflows leading to it never completing, ie. > > 214740 of 22 tuples (97%) of > pgbench_accounts done (elapsed 4038.83 s, remaining 98.93 s) > -214740 of 22 tuples (-97%) of > pgbench_accounts done (elapsed 4038.97 s, remaining -8176.86 s) > > > Attached is a patch that fixes this, pgbench -i -s 22000 works now. I think only the following line can fix this. + int64 k; Do not need to modify the type of `n`, right? -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
a On Sat, 23 Dec 2023 at 10:40, Japin Li wrote: > On Sat, 23 Dec 2023 at 08:32, Japin Li wrote: >> On Fri, 22 Dec 2023 at 23:30, Junwang Zhao wrote: >>> On Fri, Dec 22, 2023 at 10:44 PM Japin Li wrote: >>>> >>>> >>>> On Fri, 22 Dec 2023 at 22:37, Junwang Zhao wrote: >>>> > On Fri, Dec 22, 2023 at 10:25 PM Japin Li wrote: >>>> >> I try to set idle_in_transaction_session_timeout after begin >>>> >> transaction, >>>> >> it changes immediately, so I think transaction_timeout should also be >>>> >> take >>>> >> immediately. >>>> > >>>> > Ah, right, idle_in_transaction_session_timeout is set after the set >>>> > command finishes and before the backend send *ready for query* >>>> > to the client, so the value of the GUC is already set before >>>> > next command. >>>> > >>>> >>>> I mean, is it possible to set transaction_timeout before next comand? >>>> >>> Yeah, it's possible, set transaction_timeout in the when it first >>> goes into *idle in transaction* mode, see the attached files. >>> >> >> Thanks for updating the patch, LGTM. > > Sorry for the noise! > > Read the previous threads, I find why the author enable transaction_timeout > in start_xact_command(). > > The v15 patch cannot handle COMMIT AND CHAIN, see [1]. For example: > > SET transaction_timeout TO '2s'; BEGIN; SELECT 1, pg_sleep(1); COMMIT AND > CHAIN; SELECT 2, pg_sleep(1); COMMIT; > > The transaction_timeout do not reset when executing COMMIT AND CHAIN. > > [1] > https://www.postgresql.org/message-id/a906dea1-76a1-4f26-76c5-a7efad3ef5b8%40oss.nttdata.com Attach v16 to solve this. Any suggestions? -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd. >From a9d79c5a013da8fa707556f87a34ff3ade779729 Mon Sep 17 00:00:00 2001 From: "Andrey M. Borodin" Date: Sun, 3 Dec 2023 23:18:00 +0500 Subject: [PATCH v16 1/2] Introduce transaction_timeout This commit adds timeout that is expected to be used as a prevention of long-running queries. Any session within transaction will be terminated after spanning longer than this timeout. However, this timeout is not applied to prepared transactions. Only transactions with user connections are affected. Author: Andrey Borodin Reviewed-by: Nikolay Samokhvalov Reviewed-by: Andres Freund Reviewed-by: Fujii Masao Reviewed-by: bt23nguyent Reviewed-by: Yuhang Qiu Reviewed-by: Japin Li Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com --- doc/src/sgml/config.sgml | 35 +++ src/backend/postmaster/autovacuum.c | 2 + src/backend/storage/lmgr/proc.c | 1 + src/backend/tcop/postgres.c | 27 +++- src/backend/utils/errcodes.txt| 1 + src/backend/utils/init/globals.c | 1 + src/backend/utils/init/postinit.c | 10 +++ src/backend/utils/misc/guc_tables.c | 11 src/backend/utils/misc/postgresql.conf.sample | 1 + src/bin/pg_dump/pg_backup_archiver.c | 2 + src/bin/pg_dump/pg_dump.c | 2 + src/bin/pg_rewind/libpq_source.c | 1 + src/include/miscadmin.h | 1 + src/include/storage/proc.h| 1 + src/include/utils/timeout.h | 1 + src/test/isolation/Makefile | 5 +- src/test/isolation/expected/timeouts.out | 63 ++- src/test/isolation/specs/timeouts.spec| 30 + 18 files changed, 190 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index b5624ca884..d62edcf83b 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9134,6 +9134,41 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; + + transaction_timeout (integer) + + transaction_timeout configuration parameter + + + + +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. + + + +If transaction_timeout is shorter than +idle_in_transaction_session_timeout or statement_timeout +transaction_timeout will invalidate longer timeout. + + +
Re: Transaction timeout
On Sat, 23 Dec 2023 at 08:32, Japin Li wrote: > On Fri, 22 Dec 2023 at 23:30, Junwang Zhao wrote: >> On Fri, Dec 22, 2023 at 10:44 PM Japin Li wrote: >>> >>> >>> On Fri, 22 Dec 2023 at 22:37, Junwang Zhao wrote: >>> > On Fri, Dec 22, 2023 at 10:25 PM Japin Li wrote: >>> >> I try to set idle_in_transaction_session_timeout after begin transaction, >>> >> it changes immediately, so I think transaction_timeout should also be >>> >> take >>> >> immediately. >>> > >>> > Ah, right, idle_in_transaction_session_timeout is set after the set >>> > command finishes and before the backend send *ready for query* >>> > to the client, so the value of the GUC is already set before >>> > next command. >>> > >>> >>> I mean, is it possible to set transaction_timeout before next comand? >>> >> Yeah, it's possible, set transaction_timeout in the when it first >> goes into *idle in transaction* mode, see the attached files. >> > > Thanks for updating the patch, LGTM. Sorry for the noise! Read the previous threads, I find why the author enable transaction_timeout in start_xact_command(). The v15 patch cannot handle COMMIT AND CHAIN, see [1]. For example: SET transaction_timeout TO '2s'; BEGIN; SELECT 1, pg_sleep(1); COMMIT AND CHAIN; SELECT 2, pg_sleep(1); COMMIT; The transaction_timeout do not reset when executing COMMIT AND CHAIN. [1] https://www.postgresql.org/message-id/a906dea1-76a1-4f26-76c5-a7efad3ef5b8%40oss.nttdata.com -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Fri, 22 Dec 2023 at 23:30, Junwang Zhao wrote: > On Fri, Dec 22, 2023 at 10:44 PM Japin Li wrote: >> >> >> On Fri, 22 Dec 2023 at 22:37, Junwang Zhao wrote: >> > On Fri, Dec 22, 2023 at 10:25 PM Japin Li wrote: >> >> I try to set idle_in_transaction_session_timeout after begin transaction, >> >> it changes immediately, so I think transaction_timeout should also be take >> >> immediately. >> > >> > Ah, right, idle_in_transaction_session_timeout is set after the set >> > command finishes and before the backend send *ready for query* >> > to the client, so the value of the GUC is already set before >> > next command. >> > >> >> I mean, is it possible to set transaction_timeout before next comand? >> > Yeah, it's possible, set transaction_timeout in the when it first > goes into *idle in transaction* mode, see the attached files. > Thanks for updating the patch, LGTM. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: [DOC] Introducing Quick Start Guide to PL/pgSQL and PL/Python Documentation
On Thu, 21 Dec 2023 at 21:03, Pavel Stehule wrote: > Hi > > čt 21. 12. 2023 v 13:37 odesílatel Ishaan Adarsh > napsal: > >> The recent documentation patches are part of my GSoC 2023 project >> <https://wiki.postgresql.org/wiki/GSoC_2023#Postgres_extension_tutorial_.2F_quick_start> >> to develop a comprehensive PostgreSQL extension development tutorial, it >> assumes only a basic knowledge of Postgres and the target programming >> language. >> >> The entire project is available on GitHub: Postgres-extension-tutorial >> <https://github.com/IshaanAdarsh/Postgres-extension-tutorial/blob/main/SGML/intro_and_toc.md>. >> It covers many topics, including prerequisites, writing extensions, >> creating Makefiles, using procedural languages, incorporating external >> languages, writing regression tests, and managing extension releases. *The >> patch submitted >> for procedural languages, specifically PL/pgSQL and PL/Python, is part of >> the procedural language section within the broader tutorial. * >> >> Based on the feedback I think there is a real need >> <https://twitter.com/jer_s/status/1699071450915938609> for this as this >> is a very important and growing part of the Postgres ecosystem. Currently, >> all the extension material is scattered and very limited. There are >> various third-party blog posts focusing on different areas, and sometimes >> contradictory. The main motivation behind making this is to make the barrier >> for entry less prohibitive for new contributors. >> >> I would greatly appreciate your input on how to add it to the existing >> documentation (this is where I have major doubts) and any suggestions on >> how to proceed. If there are areas where the existing documentation is >> already sufficient or if there are ways to improve the overall structure, I >> am open to making adjustments. >> > > https://www.postgresql.org/docs/current/plpgsql-development-tips.html and > new section - deployment or packaging to extensions > > I agree so https://www.postgresql.org/docs/current/plpgsql-overview.html is > under dimensioned, but packaging should not be there > It seems redundant if we add this for each PL, maybe a separate section to describe how to package PL into extensions is better. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Fri, 22 Dec 2023 at 22:37, Junwang Zhao wrote: > On Fri, Dec 22, 2023 at 10:25 PM Japin Li wrote: >> I try to set idle_in_transaction_session_timeout after begin transaction, >> it changes immediately, so I think transaction_timeout should also be take >> immediately. > > Ah, right, idle_in_transaction_session_timeout is set after the set > command finishes and before the backend send *ready for query* > to the client, so the value of the GUC is already set before > next command. > I mean, is it possible to set transaction_timeout before next comand? -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Fri, 22 Dec 2023 at 20:29, Junwang Zhao wrote: > On Fri, Dec 22, 2023 at 1:39 PM Japin Li wrote: >> >> >> On Tue, 19 Dec 2023 at 22:06, Japin Li wrote: >> > On Tue, 19 Dec 2023 at 18:27, Andrey M. Borodin >> > wrote: >> >>> On 19 Dec 2023, at 13:26, Andrey M. Borodin wrote: >> >>> >> >>> I don’t have Windows machine, so I hope CF bot will pick this. >> >> >> >> I used Github CI to produce version of tests that seems to be is stable >> >> on Windows. >> > >> > It still failed on Windows Server 2019 [1]. >> > >> > diff -w -U3 C:/cirrus/src/test/isolation/expected/timeouts.out >> > C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out >> > --- C:/cirrus/src/test/isolation/expected/timeouts.out2023-12-19 >> > 10:34:30.354721100 + >> > +++ C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out >> > 2023-12-19 10:38:25.877981600 + >> > @@ -100,7 +100,7 @@ >> > step stt3_check_stt2: SELECT count(*) FROM pg_stat_activity WHERE >> > application_name = 'isolation/timeouts/stt2' >> > count >> > - >> > -0 >> > +1 >> > (1 row) >> > >> > step itt4_set: SET idle_in_transaction_session_timeout = '1ms'; SET >> > statement_timeout = '10s'; SET lock_timeout = '10s'; SET >> > transaction_timeout = '10s'; >> > >> > [1] >> > https://api.cirrus-ci.com/v1/artifact/task/4707530400595968/testrun/build/testrun/isolation/isolation/regression.diffs >> >> Hi, >> >> I try to split the test for transaction timeout, and all passed on my CI [1]. >> >> OTOH, I find if I set transaction_timeout in a transaction, it will not take >> effect immediately. For example: >> >> [local]:2049802 postgres=# BEGIN; >> BEGIN >> [local]:2049802 postgres=*# SET transaction_timeout TO '1s'; > when this execute, TransactionTimeout is still 0, this command will > not set timeout >> SET >> [local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1; -- wait >> 10s > when this command get execute, start_xact_command will enable the timer Thanks for your exaplantion, got it. >>relname >> -- >> pg_statistic >> (1 row) >> >> [local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1; >> FATAL: terminating connection due to transaction timeout >> 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: Succeeded. >> >> It looks odd. Does this is expected? I'm not read all the threads, >> am I missing something? > > I think this is by design, if you debug statement_timeout, it's the same > behaviour, the timeout will be set for each command after the second > command was called, you just aren't aware of this. > I try to set idle_in_transaction_session_timeout after begin transaction, it changes immediately, so I think transaction_timeout should also be take immediately. > I doubt people will set this in a transaction. Maybe not, -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Tue, 19 Dec 2023 at 22:06, Japin Li wrote: > On Tue, 19 Dec 2023 at 18:27, Andrey M. Borodin wrote: >>> On 19 Dec 2023, at 13:26, Andrey M. Borodin wrote: >>> >>> I don’t have Windows machine, so I hope CF bot will pick this. >> >> I used Github CI to produce version of tests that seems to be is stable on >> Windows. > > It still failed on Windows Server 2019 [1]. > > diff -w -U3 C:/cirrus/src/test/isolation/expected/timeouts.out > C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out > --- C:/cirrus/src/test/isolation/expected/timeouts.out2023-12-19 > 10:34:30.354721100 + > +++ C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out > 2023-12-19 10:38:25.877981600 + > @@ -100,7 +100,7 @@ > step stt3_check_stt2: SELECT count(*) FROM pg_stat_activity WHERE > application_name = 'isolation/timeouts/stt2' > count > - > -0 > +1 > (1 row) > > step itt4_set: SET idle_in_transaction_session_timeout = '1ms'; SET > statement_timeout = '10s'; SET lock_timeout = '10s'; SET transaction_timeout > = '10s'; > > [1] > https://api.cirrus-ci.com/v1/artifact/task/4707530400595968/testrun/build/testrun/isolation/isolation/regression.diffs Hi, I try to split the test for transaction timeout, and all passed on my CI [1]. OTOH, I find if I set transaction_timeout in a transaction, it will not take effect immediately. For example: [local]:2049802 postgres=# BEGIN; BEGIN [local]:2049802 postgres=*# SET transaction_timeout TO '1s'; SET [local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1; -- wait 10s relname -- pg_statistic (1 row) [local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1; FATAL: terminating connection due to transaction timeout 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: Succeeded. It looks odd. Does this is expected? I'm not read all the threads, am I missing something? [1] https://cirrus-ci.com/build/6574686130143232 -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd. >From fb87e5fe2ea5ced51a7e443243cdd40115423449 Mon Sep 17 00:00:00 2001 From: "Andrey M. Borodin" Date: Sun, 3 Dec 2023 23:18:00 +0500 Subject: [PATCH v13 1/1] Introduce transaction_timeout This commit adds timeout that is expected to be used as a prevention of long-running queries. Any session within transaction will be terminated after spanning longer than this timeout. However, this timeout is not applied to prepared transactions. Only transactions with user connections are affected. Author: Andrey Borodin Reviewed-by: Nikolay Samokhvalov Reviewed-by: Andres Freund Reviewed-by: Fujii Masao Reviewed-by: bt23nguyent Reviewed-by: Yuhang Qiu Reviewed-by: Japin Li Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com --- doc/src/sgml/config.sgml | 35 +++ src/backend/postmaster/autovacuum.c | 2 + src/backend/storage/lmgr/proc.c | 1 + src/backend/tcop/postgres.c | 27 +++- src/backend/utils/errcodes.txt| 1 + src/backend/utils/init/globals.c | 1 + src/backend/utils/init/postinit.c | 10 +++ src/backend/utils/misc/guc_tables.c | 11 src/backend/utils/misc/postgresql.conf.sample | 1 + src/bin/pg_dump/pg_backup_archiver.c | 2 + src/bin/pg_dump/pg_dump.c | 2 + src/bin/pg_rewind/libpq_source.c | 1 + src/include/miscadmin.h | 1 + src/include/storage/proc.h| 1 + src/include/utils/timeout.h | 1 + src/test/isolation/Makefile | 5 +- src/test/isolation/expected/timeouts.out | 63 ++- src/test/isolation/specs/timeouts.spec| 30 + 18 files changed, 190 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index b5624ca884..d62edcf83b 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9134,6 +9134,41 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; + + transaction_timeout (integer) + + transaction_timeout configuration parameter + + + + +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 milliseco
Re: [DOC] Introducing Quick Start Guide to PL/pgSQL and PL/Python Documentation
On Tue, 19 Dec 2023 at 17:57, Ishaan Adarsh wrote: > I've addressed the points you raised: > > 1. *Missing `` Tag:* > I reviewed the "Create the Makefile" section, and it seems that > tags are appropriately used for filenames. If there's a specific instance > where you observed a missing tag, please provide more details, and I'll > ensure it's addressed. > Thanks. > 2. *Use `CREATE EXTENSION` in "extension--version.sql":* > Considering that there's already a CREATE EXTENSION step in the quick > start guide, I can include a note in the general documentation to explain > the rationale without repeating it in the individual script. What do you > think? Agreed. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Tue, 19 Dec 2023 at 18:27, Andrey M. Borodin wrote: >> On 19 Dec 2023, at 13:26, Andrey M. Borodin wrote: >> >> I don’t have Windows machine, so I hope CF bot will pick this. > > I used Github CI to produce version of tests that seems to be is stable on > Windows. It still failed on Windows Server 2019 [1]. diff -w -U3 C:/cirrus/src/test/isolation/expected/timeouts.out C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out --- C:/cirrus/src/test/isolation/expected/timeouts.out 2023-12-19 10:34:30.354721100 + +++ C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out 2023-12-19 10:38:25.877981600 + @@ -100,7 +100,7 @@ step stt3_check_stt2: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/stt2' count - -0 +1 (1 row) step itt4_set: SET idle_in_transaction_session_timeout = '1ms'; SET statement_timeout = '10s'; SET lock_timeout = '10s'; SET transaction_timeout = '10s'; [1] https://api.cirrus-ci.com/v1/artifact/task/4707530400595968/testrun/build/testrun/isolation/isolation/regression.diffs -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: [DOC] Introducing Quick Start Guide to PL/pgSQL and PL/Python Documentation
On Sat, 16 Dec 2023 at 18:49, Ishaan Adarsh wrote: > Hi > > I have made some documentation enhancements for PL/pgSQL and PL/Python > sections. The changes include the addition of a Quick Start Guide to > facilitate a smoother onboarding experience for users. > > Patch File Name: > 0001-plpyhton-plpgsql-docu-changes.patch > > Patch Description: > This patch introduces a Quick Start Guide to the documentation for PL/pgSQL > and PL/Python. The Quick Start Guide provides users with a step-by-step > walkthrough of setting up and using these procedural languages. The goal is > to improve user accessibility and streamline the learning process. > > Changes Made: > 1. Added a new section titled "Quick Start Guide" to both PL/pgSQL and > PL/Python documentation. > 2. Included step-by-step instructions for users to get started with these > procedural languages. > 3. Provided explanations, code snippets, and examples to illustrate key > concepts. > > Discussion Points: > I am seeking your feedback on the following aspects: > - Clarity and completeness of the Quick Start Guide > - Any additional information or examples that could enhance the guide > - Suggestions for improving the overall documentation structure > > Your insights and suggestions are highly valuable, and I appreciate your > time in reviewing this documentation enhancement. 1. It seems you miss tag in plpgsql "Create the Makefile": + +Create the Makefile + + + Create a Makefile in the pg_plpgsql_ext directory with the following content: + 2. We expected use CREATE EXTENSION to load the extension, should we add the following in extension--version.sql? -- complain if script is sourced in psql, rather than via CREATE EXTENSION \echo Use "CREATE EXTENSION pair" to load this file. \quit -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Mon, 18 Dec 2023 at 17:40, Andrey M. Borodin wrote: >> On 18 Dec 2023, at 14:32, Japin Li wrote: >> >> >> Thanks for updating the patch > > Sorry for the noise, but commitfest bot found one more bug in handling > statement timeout. PFA v11. > On Windows, there still have an error: diff -w -U3 C:/cirrus/src/test/isolation/expected/timeouts.out C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out --- C:/cirrus/src/test/isolation/expected/timeouts.out 2023-12-18 10:22:21.772537200 + +++ C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out 2023-12-18 10:26:08.039831800 + @@ -103,24 +103,7 @@ 0 (1 row) -step stt2_check: SELECT 1; -FATAL: terminating connection due to transaction timeout -server closed the connection unexpectedly +PQconsumeInput failed: server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. -step itt4_set: SET idle_in_transaction_session_timeout = '1ms'; SET statement_timeout = '10s'; SET lock_timeout = '10s'; SET transaction_timeout = '10s'; -step itt4_begin: BEGIN ISOLATION LEVEL READ COMMITTED; -step sleep_there: SELECT pg_sleep(0.01); -pg_sleep - - -(1 row) - -step stt3_check_itt4: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/itt4' -step stt3_check_itt4: <... completed> -count -- -0 -(1 row) - -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Mon, 18 Dec 2023 at 13:49, Andrey M. Borodin wrote: >> On 16 Dec 2023, at 05:58, Japin Li wrote: >> >> >> On Fri, 15 Dec 2023 at 17:51, Andrey M. Borodin wrote: >>>> On 8 Dec 2023, at 15:29, Japin Li wrote: >>>> >>>> Thanks for updating the patch. LGTM. >>> >>> PFA v9. Changes: >>> 1. Added tests for idle_in_transaction_timeout >>> 2. Suppress statement_timeout if it’s shorter than transaction_timeout >>> >> + if (StatementTimeout > 0 >> + && IdleInTransactionSessionTimeout < TransactionTimeout) >> ^ >> >> Should be StatementTimeout? > Yes, that’s an oversight. I’ve adjusted tests so they catch this problem. > >> Maybe we should add documentation to describe this behavior. > > I've added a paragraph about it to config.sgml, but I'm not sure about the > comprehensiveness of the wording. > Thanks for updating the patch, no objections. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Fri, 15 Dec 2023 at 17:51, Andrey M. Borodin wrote: >> On 8 Dec 2023, at 15:29, Japin Li wrote: >> >> Thanks for updating the patch. LGTM. > > PFA v9. Changes: > 1. Added tests for idle_in_transaction_timeout > 2. Suppress statement_timeout if it’s shorter than transaction_timeout > + if (StatementTimeout > 0 + && IdleInTransactionSessionTimeout < TransactionTimeout) ^ Should be StatementTimeout? Maybe we should add documentation to describe this behavior. > Consider changing status of the commitfest entry if you think it’s ready for > committer. > -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Fri, 08 Dec 2023 at 18:08, Andrey M. Borodin wrote: >> On 8 Dec 2023, at 12:59, Japin Li wrote: >> >> >> On Thu, 07 Dec 2023 at 20:40, Andrey M. Borodin wrote: >>>> On 7 Dec 2023, at 06:25, Japin Li wrote: >>>> >>>> If idle_in_transaction_timeout is bigger than transaction_timeout, >>>> the idle-in-transaction timeout don't needed, right? >>> Yes, I think so. >>> >> >> Should we disable the idle_in_transaction_timeout in this case? Of cursor, >> I'm >> not strongly insist on this. > Good idea! > >> I think you forget disable transaction_timeout in AutoVacWorkerMain(). >> If not, can you elaborate on why you don't disable it? > > Seems like code in autovacuum.c was copied, but patch was not updated. I’ve > fixed this oversight. > Thanks for updating the patch. LGTM. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Thu, 07 Dec 2023 at 20:40, Andrey M. Borodin wrote: >> On 7 Dec 2023, at 06:25, Japin Li wrote: >> >> If idle_in_transaction_timeout is bigger than transaction_timeout, >> the idle-in-transaction timeout don't needed, right? > Yes, I think so. > Should we disable the idle_in_transaction_timeout in this case? Of cursor, I'm not strongly insist on this. I think you forget disable transaction_timeout in AutoVacWorkerMain(). If not, can you elaborate on why you don't disable it? -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Transaction timeout
On Wed, 06 Dec 2023 at 21:05, Andrey M. Borodin wrote: >> On 30 Nov 2023, at 20:06, Andrey M. Borodin wrote: >> >> >> Tomorrow I plan to fix raising of the timeout when the transaction is idle. >> Renaming transaction_timeout to something else (to avoid confusion with >> prepared xacts) also seems correct to me. > > > Here's a v6 version of the feature. Changes: > 1. Now transaction_timeout will break connection with FATAL instead of > hanging in "idle in transaction (aborted)" > 2. It will kill equally idle and active transactions > 3. New isolation tests are slightly more complex: isolation tester does not > like when the connection is forcibly killed, thus there must be only 1 > permutation with killed connection. > Greate. If idle_in_transaction_timeout is bigger than transaction_timeout, the idle-in-transaction timeout don't needed, right? > TODO: as Yuhang pointed out prepared transactions must not be killed, thus > name "transaction_timeout" is not correct. I think the name must be like > "session_transaction_timeout", but I'd like to have an opinion of someone > more experienced in giving names to GUCs than me. Or, perhaps, a native > speaker? > How about transaction_session_timeout? Similar to idle_session_timeout. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Tab completion regression test failed on illumos
On Fri, 03 Nov 2023 at 10:03, Thomas Munro wrote: > On Fri, Nov 3, 2023 at 2:22 PM Thomas Munro wrote: >> On Fri, Nov 3, 2023 at 3:42 AM Japin Li wrote: >> > I think this might be a bug comes from Illumos pseudo-tty. I can reproduce >> > this by using pseudo-tty on Illumos. >> >> I don't know but my guess is that this has to do with termios defaults >> being different. From a quick look at 'man termios', perhaps TABDLY >> is set to expand tabs to spaces? Can you fix it by tweaking the flags >> in src/common/sprompt.c? > > Argh, sorry that's completely the wrong end. I suppose that sort of > thing would have to happen in IPC::Run. I wonder what would happen if > IPC::Run called ->set_raw() on the IO::Pty object it constructs, or > failing that, if IO::Stty can be used to mess with the relevant > settings. Thanks for your explantation, the termios.c_oflag on Illumos enables TABDLY by default. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Tab completion regression test failed on illumos
On Thu, 02 Nov 2023 at 22:23, Tom Lane wrote:
> Japin Li writes:
>> It seems the 'SEL\t' is converted to 'SEL ' which is "SEL" with 5 spaces.
>
> That would be plausible if readline were disabled, or otherwise
> not functioning.
>
I think this might be a bug comes from Illumos pseudo-tty. I can reproduce
this by using pseudo-tty on Illumos.
Here is a simple test case:
$ cat pseudo-tty.c
#define _XOPEN_SOURCE 600
#include
#include
#include
#include
#include
#include
#include
#define DEV_PTMX"/dev/ptmx"
int
main(void)
{
int ptm_fd;
pid_t pid;
char *pts_name;
ptm_fd = open(DEV_PTMX, O_RDWR);
grantpt(ptm_fd);
unlockpt(ptm_fd);
pts_name = ptsname(ptm_fd);
pid = fork();
if (pid == -1) {
fprintf(stderr, "could not fork a new process: %m\n");
close(ptm_fd);
return -1;
} else if (pid == 0) {
int pts_fd;
close(ptm_fd);
pts_fd = open(pts_name, O_RDWR);
write(pts_fd, "SEL\tH", 5);
close(pts_fd);
} else {
int status;
char buffer[512] = { 0 };
ssize_t bytes;
bytes = read(ptm_fd, buffer, sizeof(buffer));
printf("%ld: '%s'\n", bytes, buffer);
waitpid(pid, , 0);
close(ptm_fd);
}
return 0;
}
On IllumsOS
$ gcc -o pseudo-tty pseudo-tty.c
$ ./pseudo-tty
9: 'SEL H'
On Ubuntu
$ gcc -o pseudo-tty pseudo-tty.c
$ ./pseudo-tty
5: 'SEL H'
--
Regrads,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
Re: Tab completion regression test failed on illumos
On Thu, 02 Nov 2023 at 13:42, Japin Li wrote: > On Thu, 02 Nov 2023 at 13:01, Noah Misch wrote: >> On Wed, Nov 01, 2023 at 03:19:39PM +0800, Japin Li wrote: >>> I try to run regression test on illumos, the 010_tab_completion will >>> failed because of timeout. >> >>> Any suggestions? Thanks in advance! >> >> This test failed for me, in a different way, when I briefly installed IO::Pty >> on a Solaris buildfarm member: >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=wrasse=2023-01-03%2022%3A39%3A26 >> > Thanks for confirm this! > > I try to install IO::Pty using cpan, however, I cannot get the same error. > >> The IPC::Run pty tests also fail on Solaris. If I were debugging this, I'd >> start by fixing IO::Pty and IPC::Run to pass their own test suites on Solaris >> or illumos. Then I'd see if problems continue for this postgresql test. > > So, it might be a bug comes from Perl. After enable debug for IPC::Run, I found the following logs: IPC::Run 0001 0123456789012-4 [#2(415745)]: writing to fd 11 (kid's stdin) IPC::Run 0001 0123456789012-4 [#2(415745)]: write( 11, 'SEL ' ) = 4 <- Here write 4 bytes. IPC::Run 0001 0123456789012-4 [#2(415745)]: fds for select: ---r--r IPC::Run 0001 0123456789012-4 [#2(415745)]: timeout=0 IPC::Run 0001 0123456789012-4 [#2(415745)]: selected ---r IPC::Run 0001 0123456789012-4 [#2(415745)]: filtering data from fd 11 (kid's stdout) IPC::Run 0001 0123456789012-4 [#2(415745)]: reading from fd 11 (kid's stdout) IPC::Run 0001 0123456789012-4 [#2(415745)]: read( 11 ) = 8 chars 'SEL ' <- But read 8 bytes. It seems the 'SEL\t' is converted to 'SEL ' which is "SEL" with 5 spaces. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Tab completion regression test failed on illumos
On Thu, 02 Nov 2023 at 13:01, Noah Misch wrote: > On Wed, Nov 01, 2023 at 03:19:39PM +0800, Japin Li wrote: >> I try to run regression test on illumos, the 010_tab_completion will >> failed because of timeout. > >> Any suggestions? Thanks in advance! > > This test failed for me, in a different way, when I briefly installed IO::Pty > on a Solaris buildfarm member: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=wrasse=2023-01-03%2022%3A39%3A26 > Thanks for confirm this! I try to install IO::Pty using cpan, however, I cannot get the same error. > The IPC::Run pty tests also fail on Solaris. If I were debugging this, I'd > start by fixing IO::Pty and IPC::Run to pass their own test suites on Solaris > or illumos. Then I'd see if problems continue for this postgresql test. So, it might be a bug comes from Perl. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Tab completion regression test failed on illumos
Hi hackers,
I try to run regression test on illumos, the 010_tab_completion will
failed because of timeout.
Here is my build commands and logs:
$ ../configure --enable-cassert --enable-debug --enable-nls --with-perl \
--with-python --with-tcl --with-openssl --with-libxml --with-libxslt \
--without-icu --enable-tap-tests --prefix=/home/japin/postgres/build/pg
$ make -j $(nproc)
...
$ cd src/bin/psql/ && make check
make -C ../../../src/backend generated-headers
make[1]: Entering directory '/home/japin/postgres/build/src/backend'
make -C catalog distprep generated-header-symlinks
make[2]: Entering directory '/home/japin/postgres/build/src/backend/catalog'
make[2]: Nothing to be done for 'distprep'.
make[2]: Nothing to be done for 'generated-header-symlinks'.
make[2]: Leaving directory '/home/japin/postgres/build/src/backend/catalog'
make -C nodes distprep generated-header-symlinks
make[2]: Entering directory '/home/japin/postgres/build/src/backend/nodes'
make[2]: Nothing to be done for 'distprep'.
make[2]: Nothing to be done for 'generated-header-symlinks'.
make[2]: Leaving directory '/home/japin/postgres/build/src/backend/nodes'
make -C utils distprep generated-header-symlinks
make[2]: Entering directory '/home/japin/postgres/build/src/backend/utils'
make[2]: Nothing to be done for 'distprep'.
make[2]: Nothing to be done for 'generated-header-symlinks'.
make[2]: Leaving directory '/home/japin/postgres/build/src/backend/utils'
make[1]: Leaving directory '/home/japin/postgres/build/src/backend'
rm -rf '/home/japin/postgres/build'/tmp_install
/opt/local/bin/mkdir -p '/home/japin/postgres/build'/tmp_install/log
make -C '../../..' DESTDIR='/home/japin/postgres/build'/tmp_install install
>'/home/japin/postgres/build'/tmp_install/log/install.log 2>&1
make -j1 checkprep
>>'/home/japin/postgres/build'/tmp_install/log/install.log 2>&1
PATH="/home/japin/postgres/build/tmp_install/home/japin/postgres/build/pg/bin:/home/japin/postgres/build/src/bin/psql:$PATH"
LD_LIBRARY_PATH="/home/japin/postgres/build/tmp_install/home/japin/postgres/build/pg/lib"
INITDB_TEMPLATE='/home/japin/postgres/build'/tmp_install/initdb-template
initdb -A trust -N --no-instructions --no-locale
'/home/japin/postgres/build'/tmp_install/initdb-template
>>'/home/japin/postgres/build'/tmp_install/log/initdb-template.log 2>&1
echo "# +++ tap check in src/bin/psql +++" && rm -rf
'/home/japin/postgres/build/src/bin/psql'/tmp_check && /opt/local/bin/mkdir -p
'/home/japin/postgres/build/src/bin/psql'/tmp_check && cd
/home/japin/postgres/build/../src/bin/psql &&
TESTLOGDIR='/home/japin/postgres/build/src/bin/psql/tmp_check/log'
TESTDATADIR='/home/japin/postgres/build/src/bin/psql/tmp_check'
PATH="/home/japin/postgres/build/tmp_install/home/japin/postgres/build/pg/bin:/home/japin/postgres/build/src/bin/psql:$PATH"
LD_LIBRARY_PATH="/home/japin/postgres/build/tmp_install/home/japin/postgres/build/pg/lib"
INITDB_TEMPLATE='/home/japin/postgres/build'/tmp_install/initdb-template
PGPORT='65432' top_builddir='/home/japin/postgres/build/src/bin/psql/../../..'
PG_REGRESS='/home/japin/postgres/build/src/bin/psql/../../../src/test/regress/pg_regress'
/opt/local/bin/prove -I /home/japin/postgres/build/../src/test/perl/ -I
/home/japin/postgres/build/../src/bin/psql t/*.pl
# +++ tap check in src/bin/psql +++
t/001_basic.pl ... ok
t/010_tab_completion.pl .. Dubious, test returned 25 (wstat 6400, 0x1900)
No subtests run
t/020_cancel.pl .. ok
Test Summary Report
---
t/010_tab_completion.pl (Wstat: 6400 Tests: 0 Failed: 0)
Non-zero exit status: 25
Parse errors: No plan found in TAP output
$ cat tmp_check/log/regress_log_010_tab_completion
# Checking port 59378
# Found port 59378
Name: main
Data directory:
/home/japin/postgres/build/src/bin/psql/tmp_check/t_010_tab_completion_main_data/pgdata
Backup directory:
/home/japin/postgres/build/src/bin/psql/tmp_check/t_010_tab_completion_main_data/backup
Archive directory:
/home/japin/postgres/build/src/bin/psql/tmp_check/t_010_tab_completion_main_data/archives
Connection string: port=59378 host=/tmp/2tdG0Ck7Zb
Log file:
/home/japin/postgres/build/src/bin/psql/tmp_check/log/010_tab_completion_main.log
[07:06:06.492](0.050s) # initializing database system by copying initdb
template
# Running: cp -RPp /home/japin/postgres/build/tmp_install/initdb-template
/home/japin/postgres/build/src/bin/psql/tmp_check/t_010_tab_completion_main_data/pgdata
# Running:
/home/japin/postgres/build/src/bin/psql/../../../src/test/regress/pg_regress
--config-auth
/home/japin/postgres/build/src/bin/psql/tmp_check/t_010_tab_completio
Re: Could not run generate_unaccent_rules.py script when update unicode
On Wed, 27 Sep 2023 at 13:46, Michael Paquier wrote: > On Wed, Sep 27, 2023 at 09:15:00AM +0800, Japin Li wrote: >> On Wed, 27 Sep 2023 at 08:03, Michael Paquier wrote: >>> I am not sure that many people run this script frequently so that may >>> not be worth adding a check for a defined, still empty or incorrect >> >> Yeah, not frequently, however, it already be used by me, since we provide >> this >> function, why not make it better? > > Well, I don't mind doing as you suggest. > >>> value, but.. If you were to change the Makefile we use in this path, >>> how are you suggesting to change it? >> >> I provide a patch at bottom of in [1]. Attached here again. > > No file was attached so I somewhat missed it. And indeed, you're > right. The current rule is useless when compiling without > --with-python, as PYTHON is empty but defined. What you are proposing > is similar to what pgxs.mk does for bison and flex, and "python" would > still be able to map to python3 or python2.7, which should be OK in > most cases. > > I thought that this was quite old, but no, f85a485f8 was at the origin > of that. So I've applied the patch down to 13. Thanks for your review and push! -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Could not run generate_unaccent_rules.py script when update unicode
On Wed, 27 Sep 2023 at 08:03, Michael Paquier wrote: > On Tue, Sep 26, 2023 at 10:43:40AM +0800, Japin Li wrote: >> # Allow running this even without --with-python >> PYTHON ?= python >> >> $(srcdir)/unaccent.rules: generate_unaccent_rules.py >> ../../src/common/unicode/UnicodeData.txt Latin-ASCII.xml >> $(PYTHON) $< --unicode-data-file $(word 2,$^) --latin-ascii-file >> $(word 3,$^) >$@ >> >> It use python to run generate_unaccent_rules.py, However, the ?= operator in >> Makefile only check variable is defined or not, but do not check variable is >> empty. Since the PYTHON is defined in src/Makefile.global, so here PYTHON >> get empty when without --with-ptyhon. > > I am not sure that many people run this script frequently so that may > not be worth adding a check for a defined, still empty or incorrect Yeah, not frequently, however, it already be used by me, since we provide this function, why not make it better? > value, but.. If you were to change the Makefile we use in this path, > how are you suggesting to change it? I provide a patch at bottom of in [1]. Attached here again. diff --git a/contrib/unaccent/Makefile b/contrib/unaccent/Makefile index 652a3e774c..3ff49ba1e9 100644 --- a/contrib/unaccent/Makefile +++ b/contrib/unaccent/Makefile @@ -26,7 +26,9 @@ endif update-unicode: $(srcdir)/unaccent.rules # Allow running this even without --with-python -PYTHON ?= python +ifeq ($(PYTHON),) +PYTHON = python +endif $(srcdir)/unaccent.rules: generate_unaccent_rules.py ../../src/common/unicode/UnicodeData.txt Latin-ASCII.xml $(PYTHON) $< --unicode-data-file $(word 2,$^) --latin-ascii-file $(word 3,$^) >$@ [1] https://www.postgresql.org/message-id/meyp282mb1669f86c0dc7b4dc48489cb0b6...@meyp282mb1669.ausp282.prod.outlook.com -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Could not run generate_unaccent_rules.py script when update unicode
Hi, hackers When I try to update unicode mapping tables using make update-unicode [1], I encountered an error about following: generate_unaccent_rules.py --unicode-data-file ../../src/common/unicode/UnicodeData.txt --latin-ascii-file Latin-ASCII.xml >unaccent.rules /bin/sh: 1: generate_unaccent_rules.py: not found make: *** [Makefile:33: unaccent.rules] Error 127 make: *** Deleting file 'unaccent.rules' The generate_unaccent_rules.py is in contrib/unaccent and the Makefile: # Allow running this even without --with-python PYTHON ?= python $(srcdir)/unaccent.rules: generate_unaccent_rules.py ../../src/common/unicode/UnicodeData.txt Latin-ASCII.xml $(PYTHON) $< --unicode-data-file $(word 2,$^) --latin-ascii-file $(word 3,$^) >$@ It use python to run generate_unaccent_rules.py, However, the ?= operator in Makefile only check variable is defined or not, but do not check variable is empty. Since the PYTHON is defined in src/Makefile.global, so here PYTHON get empty when without --with-ptyhon. Here are some examples: japin@coltd-devel:~$ cat Makefile PYTHON = PYTHON ?= python test: echo '$(PYTHON)' japin@coltd-devel:~$ make echo '' japin@coltd-devel:~$ cat Makefile PYTHON = python3 PYTHON ?= python test: echo '$(PYTHON)' japin@coltd-devel:~$ make echo 'python3' python3 japin@coltd-devel:~$ cat Makefile PYTHON = ifeq ($(PYTHON),) PYTHON = python endif test: echo '$(PYTHON)' japin@coltd-devel:~$ make echo 'python' python japin@coltd-devel:~$ cat Makefile PYTHON = python3 ifeq ($(PYTHON),) PYTHON = python endif test: echo '$(PYTHON)' japin@coltd-devel:~$ make echo 'python3' python3 Here is a patch to fix this, any thoughts? diff --git a/contrib/unaccent/Makefile b/contrib/unaccent/Makefile index 652a3e774c..3ff49ba1e9 100644 --- a/contrib/unaccent/Makefile +++ b/contrib/unaccent/Makefile @@ -26,7 +26,9 @@ endif update-unicode: $(srcdir)/unaccent.rules # Allow running this even without --with-python -PYTHON ?= python +ifeq ($(PYTHON),) +PYTHON = python +endif $(srcdir)/unaccent.rules: generate_unaccent_rules.py ../../src/common/unicode/UnicodeData.txt Latin-ASCII.xml $(PYTHON) $< --unicode-data-file $(word 2,$^) --latin-ascii-file $(word 3,$^) >$@ [1] https://www.postgresql.org/message-id/MEYP282MB1669AC78EE8374B3DE797A09B6FCA%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: How to update unicode mapping table?
On Tue, 26 Sep 2023 at 06:20, Tom Lane wrote: > Peter Eisentraut writes: >> On 25.09.23 08:02, Japin Li wrote: >>> When I try to update the unicode mapping table through *.xml in >>> src/backend/utils/mb/Unicode/, it doesn't update the *.map files. >>> I find the make cannot go to this directory, what can I do to update >>> the mapping tables? > >> This is done by "make update-unicode". > > On a slightly related note, I noticed while preparing 3aff1d3fd > that src/backend/utils/mb/Unicode/Makefile seems a little screwy. > > So there doesn't seem to be any clean way to regenerate the fileset > present in git. Maybe these targets aren't supposed to be invoked > here, but then why have a Makefile here at all? Alternatively, > maybe we have files in git that shouldn't be there (very likely due > to the fact that this directory also lacks a .gitignore file). > I find those files do not removed when using VPATH build. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: How to update unicode mapping table?
On Tue, 26 Sep 2023 at 05:58, Peter Eisentraut wrote: > On 25.09.23 08:02, Japin Li wrote: >> When I try to update the unicode mapping table through *.xml in >> src/backend/utils/mb/Unicode/, it doesn't update the *.map files. >> I find the make cannot go to this directory, what can I do to update >> the mapping tables? > > This is done by "make update-unicode". Thanks, it seems works. -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
How to update unicode mapping table?
Hi, hackers When I try to update the unicode mapping table through *.xml in src/backend/utils/mb/Unicode/, it doesn't update the *.map files. I find the make cannot go to this directory, what can I do to update the mapping tables? -- Regrads, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Re: Confused about gram.y referencs in Makefile?
On Mon, 25 Sep 2023 at 11:17, Tom Lane wrote: > Japin Li writes: >> I find src/backend/utils/mb/Unicode/Makefile has the following comments: > >>> # Note that while each script call produces two output files, to be >>> # parallel-make safe we need to split this into two rules. (See for >>> # example gram.y for more explanation.) > >> I could not find the explanation in gram.y easily. Would someone point >> it out for me? Thanks in advance! > > It's referring to this bit in src/backend/parser/Makefile: > > - > # There is no correct way to write a rule that generates two files. > # Rules with two targets don't have that meaning, they are merely > # shorthand for two otherwise separate rules. If we have an action > # that in fact generates two or more files, we must choose one of them > # as primary and show it as the action's output, then make all of the > # other output files dependent on the primary, like this. Furthermore, > # the "touch" action is essential, because it ensures that gram.h is > # marked as newer than (or at least no older than) gram.c. Without that, > # make is likely to try to rebuild gram.h in subsequent runs, which causes > # failures in VPATH builds from tarballs. > > gram.h: gram.c > touch $@ > > gram.c: BISONFLAGS += -d > gram.c: BISON_CHECK_CMD = $(PERL) $(srcdir)/check_keywords.pl $< > $(top_srcdir)/src/include/parser/kwlist.h > - > > This is indeed kind of confusing, because there's no explicit > reference to gram.y here --- the last two lines just tweak > the behavior of the default .y to .c rule. > > Maybe we should adjust the comment in Unicode/Makefile, but > I'm not sure what would be a better reference. > > regards, tom lane Thank you! Maybe be reference src/backend/parser/Makefile is better than current. How about "See gram.h target's comment in src/backend/parser/Makefile" or just "See src/backend/parser/Makefile"? -- Regrads, Japin Li
Confused about gram.y referencs in Makefile?
Hi, hackers I find src/backend/utils/mb/Unicode/Makefile has the following comments: > # Note that while each script call produces two output files, to be > # parallel-make safe we need to split this into two rules. (See for > # example gram.y for more explanation.) > # I could not find the explanation in gram.y easily. Would someone point it out for me? Thanks in advance! -- Regrads, Japin Li
Confused about gram.y referencs in Makefile?
Hi, hackers I find src/backend/utils/mb/Unicode/Makefile has the following comments: > # Note that while each script call produces two output files, to be > # parallel-make safe we need to split this into two rules. (See for > # example gram.y for more explanation.) > # I could not find the explanation in gram.y easily. Would someone point it out for me? Thanks in advance! -- Regrads, Japin Li
Re: Regression test collate.icu.utf8 failed on REL_14_STABLE
On Tue, 15 Aug 2023 at 08:54, Japin Li wrote: > On Tue, 15 Aug 2023 at 08:49, Andy Fan wrote: >>> >>> >>> >>> DROP SCHEMA test_schema; >>> +ERROR: cannot drop schema test_schema because other objects depend on it >>> +DETAIL: collation test_schema.test11 depends on schema test_schema >>> +HINT: Use DROP ... CASCADE to drop the dependent objects too. >>> DROP ROLE regress_test_role; >>> +ERROR: role "regress_test_role" cannot be dropped because some objects >>> depend on it >>> +DETAIL: owner of collation test_schema.test11 >>> +ERROR: role "regress_test_role" cannot be dropped because some objects >>> depend on it >>> +DETAIL: owner of collation test_schema.test11 >>> >>> +ERROR: role "regress_test_role" already exists >>> >>> >>> >>> >> Did you run 'make installcheck' rather than 'make check' and there >> was a failure before this round of test? This looks to me that there >> are some objects are not cleaned well before this run. you can try >> 'make installcheck' with a pretty clean setup or run 'make check' >> directly to verify this. > Thanks Andy, I think I find the root cause. In my environment, LANG has different setting from others. $ locale LANG=C.UTF-8 LANGUAGE= LC_CTYPE="en_US.UTF-8" LC_NUMERIC="en_US.UTF-8" LC_TIME="en_US.UTF-8" LC_COLLATE="en_US.UTF-8" LC_MONETARY="en_US.UTF-8" LC_MESSAGES="en_US.UTF-8" LC_PAPER="en_US.UTF-8" LC_NAME="en_US.UTF-8" LC_ADDRESS="en_US.UTF-8" LC_TELEPHONE="en_US.UTF-8" LC_MEASUREMENT="en_US.UTF-8" LC_IDENTIFICATION="en_US.UTF-8" LC_ALL=en_US.UTF-8 Then, I set LANG to en_US.UTF-8, all tests passed. What I'm curious about is why PG 15 can pass. -- Regrads, Japin Li
Re: Regression test collate.icu.utf8 failed on REL_14_STABLE
On Tue, 15 Aug 2023 at 08:49, Andy Fan wrote: >> >> >> >> DROP SCHEMA test_schema; >> +ERROR: cannot drop schema test_schema because other objects depend on it >> +DETAIL: collation test_schema.test11 depends on schema test_schema >> +HINT: Use DROP ... CASCADE to drop the dependent objects too. >> DROP ROLE regress_test_role; >> +ERROR: role "regress_test_role" cannot be dropped because some objects >> depend on it >> +DETAIL: owner of collation test_schema.test11 >> +ERROR: role "regress_test_role" cannot be dropped because some objects >> depend on it >> +DETAIL: owner of collation test_schema.test11 >> >> +ERROR: role "regress_test_role" already exists >> >> >> >> > Did you run 'make installcheck' rather than 'make check' and there > was a failure before this round of test? This looks to me that there > are some objects are not cleaned well before this run. you can try > 'make installcheck' with a pretty clean setup or run 'make check' > directly to verify this. I used `make check` and cleanup the entire build directory. Here is my compile & build script. $ cat compile.sh #!/bin/bash set -e rm -rf $(ls -I '*.sh') ../configure \ --prefix=$PWD/pg \ --enable-tap-tests \ --enable-debug \ --enable-cassert \ --enable-depend \ --enable-dtrace \ --with-icu \ --with-llvm \ --with-openssl \ --with-python \ --with-libxml \ --with-libxslt \ --with-lz4 \ --with-pam \ CFLAGS='-O0 -Wmissing-prototypes -Wincompatible-pointer-types' \ >configure.log 2>&1 make -j $(nproc) -s && make install -s (cd contrib/ && make -j $(nproc) -s && make install -s) -- Regrads, Japin Li
Regression test collate.icu.utf8 failed on REL_14_STABLE
Hi, hackers
I find when I compile PG 14 with --with-icu, collate.icu.utf8 and foreign_data
regression tests will failed. However it is OK on REL_15_STABLE and master.
I also test this on REL_13_STABLE, and it also failed. Here is the regression
diffs.
diff -U3
/home/japin/Codes/postgres/build/../src/test/regress/expected/collate.icu.utf8.out
/home/japin/Codes/postgres/build/src/test/regress/results/collate.icu.utf8.out
---
/home/japin/Codes/postgres/build/../src/test/regress/expected/collate.icu.utf8.out
2023-08-14 17:37:31.960448245 +0800
+++
/home/japin/Codes/postgres/build/src/test/regress/results/collate.icu.utf8.out
2023-08-14 21:30:44.335214886 +0800
@@ -1035,6 +1035,9 @@
quote_literal(current_setting('lc_ctype')) || ');';
END
$$;
+ERROR: collations with different collate and ctype values are not supported
by ICU
+CONTEXT: SQL statement "CREATE COLLATION test1 (provider = icu, lc_collate =
'C.UTF-8', lc_ctype = 'en_US.UTF-8');"
+PL/pgSQL function inline_code_block line 3 at EXECUTE
CREATE COLLATION test3 (provider = icu, lc_collate = 'en_US.utf8'); -- fail,
need lc_ctype
ERROR: parameter "lc_ctype" must be specified
CREATE COLLATION testx (provider = icu, locale = 'nonsense'); /* never fails
with ICU */ DROP COLLATION testx;
@@ -1045,13 +1048,12 @@
collname
--
test0
- test1
test5
-(3 rows)
+(2 rows)
ALTER COLLATION test1 RENAME TO test11;
+ERROR: collation "test1" for encoding "UTF8" does not exist
ALTER COLLATION test0 RENAME TO test11; -- fail
-ERROR: collation "test11" already exists in schema "collate_tests"
ALTER COLLATION test1 RENAME TO test22; -- fail
ERROR: collation "test1" for encoding "UTF8" does not exist
ALTER COLLATION test11 OWNER TO regress_test_role;
@@ -1059,18 +1061,19 @@
ERROR: role "nonsense" does not exist
ALTER COLLATION test11 SET SCHEMA test_schema;
COMMENT ON COLLATION test0 IS 'US English';
+ERROR: collation "test0" for encoding "UTF8" does not exist
SELECT collname, nspname, obj_description(pg_collation.oid, 'pg_collation')
FROM pg_collation JOIN pg_namespace ON (collnamespace = pg_namespace.oid)
WHERE collname LIKE 'test%'
ORDER BY 1;
collname |nspname| obj_description
--+---+-
- test0| collate_tests | US English
test11 | test_schema |
test5| collate_tests |
-(3 rows)
+(2 rows)
DROP COLLATION test0, test_schema.test11, test5;
+ERROR: collation "test0" for encoding "UTF8" does not exist
DROP COLLATION test0; -- fail
ERROR: collation "test0" for encoding "UTF8" does not exist
DROP COLLATION IF EXISTS test0;
@@ -1078,10 +1081,17 @@
SELECT collname FROM pg_collation WHERE collname LIKE 'test%';
collname
--
-(0 rows)
+ test11
+ test5
+(2 rows)
DROP SCHEMA test_schema;
+ERROR: cannot drop schema test_schema because other objects depend on it
+DETAIL: collation test_schema.test11 depends on schema test_schema
+HINT: Use DROP ... CASCADE to drop the dependent objects too.
DROP ROLE regress_test_role;
+ERROR: role "regress_test_role" cannot be dropped because some objects depend
on it
+DETAIL: owner of collation test_schema.test11
-- ALTER
ALTER COLLATION "en-x-icu" REFRESH VERSION;
NOTICE: version has not changed
diff -U3
/home/japin/Codes/postgres/build/../src/test/regress/expected/foreign_data.out
/home/japin/Codes/postgres/build/src/test/regress/results/foreign_data.out
---
/home/japin/Codes/postgres/build/../src/test/regress/expected/foreign_data.out
2023-08-14 17:37:31.964448260 +0800
+++ /home/japin/Codes/postgres/build/src/test/regress/results/foreign_data.out
2023-08-14 21:30:55.571170376 +0800
@@ -5,10 +5,13 @@
-- Suppress NOTICE messages when roles don't exist
SET client_min_messages TO 'warning';
DROP ROLE IF EXISTS regress_foreign_data_user, regress_test_role,
regress_test_role2, regress_test_role_super, regress_test_indirect,
regress_unprivileged_role;
+ERROR: role "regress_test_role" cannot be dropped because some objects depend
on it
+DETAIL: owner of collation test_schema.test11
RESET client_min_messages;
CREATE ROLE regress_foreign_data_user LOGIN SUPERUSER;
SET SESSION AUTHORIZATION 'regress_foreign_data_user';
CREATE ROLE regress_test_role;
+ERROR: role "regress_test_role" already exists
CREATE ROLE regress_test_role2;
CREATE ROLE regress_test_role_super SUPERUSER;
CREATE ROLE regress_test_indirect;
Is it a bug that fixed in REL_15_STABLE? If yes, why not backpatch?
--
Regrads,
Japin Li
Re: Add hint message for check_log_destination()
On Thu, 13 Jul 2023 at 16:19, Masahiko Sawada wrote:
> On Tue, Jul 11, 2023 at 10:24 AM Japin Li wrote:
>>
>>
>> On Mon, 10 Jul 2023 at 14:23, Masahiko Sawada wrote:
>> > On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
>> > wrote:
>> >>
>> >> At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li wrote
>> >> in
>> >> >
>> >> > On Sat, 08 Jul 2023 at 12:48, Michael Paquier
>> >> > wrote:
>> >> > > On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
>> >> > >> + appendStringInfoString(, "\"stderr\"");
>> >> > >> +#ifdef HAVE_SYSLOG
>> >> > >> + appendStringInfoString(, ", \"syslog\"");
>> >> > >> +#endif
>> >> > >> +#ifdef WIN32
>> >> > >> + appendStringInfoString(, ",
>> >> > >> \"eventlog\"");
>> >> > >> +#endif
>> >> > >> + appendStringInfoString(, ", \"csvlog\",
>> >> > >> and \"jsonlog\"");
>> >> > >
>> >> > > Hmm. Is that OK as a translatable string?
>> >
>> > It seems okay to me but needs to be checked.
>> >
>> >> >
>> >> >
>> >> > Sorry for the late reply! I'm not sure. How can I know whether it is
>> >> > translatable?
>> >>
>> >> At the very least, we can't generate comma-separated lists
>> >> programatically because punctuation marks vary across languages.
>> >>
>> >> One potential approach could involve defining the message for every
>> >> potential combination, in full length.
>> >
>> > Don't we generate a comma-separated list for an error hint of an enum
>> > parameter? For example, to generate the following error hint:
>> >
>> > =# alter system set client_min_messages = 'aaa';
>> > ERROR: invalid value for parameter "client_min_messages": "aaa"
>> > HINT: Available values: debug5, debug4, debug3, debug2, debug1, log,
>> > notice, warning, error.
>> >
>> > we use the comma-separated generated by config_enum_get_options() and
>> > do ereport() like:
>> >
>> > ereport(elevel,
>> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> >errmsg("invalid value for parameter \"%s\": \"%s\"",
>> > name, value),
>> >hintmsg ? errhint("%s", _(hintmsg)) : 0));
>>
>> > IMO log_destination is a string GUC parameter but its value is the
>> > list of enums. So it makes sense to me to add a hint message like what
>> > we do for enum parameters in case where the user mistypes a wrong
>> > value. I'm not sure why the proposed patch needs to quote the usable
>> > values, though.
>>
>> I borrowed the description from pg_settings extra_desc. In my first patch,
>> I used the hint message line enum parameter, however, since it might be a
>> combination of multiple log destinations, so, I update the patch using
>> extra_desc.
>
> I agree to use description from pg_settings extra_desc, but it seems
> to be better not to quote each available value like we do for enum
> parameter cases. That is, the hint message would be like:
>
> =# alter system set log_destination to 'xxx';
> ERROR: invalid value for parameter "log_destination": "xxx"
> DETAIL: Unrecognized key word: "xxx".
> HINT: Valid values are combinations of stderr, syslog, csvlog, and jsonlog.
>
Agreed. Fixed as per your suggestions.
--
Regrads,
Japin Li
>From 3f709effe14de81b84bbc441aa5884ece0a757da Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Fri, 14 Jul 2023 09:26:01 +0800
Subject: [PATCH v4 1/1] Add hint message for check_log_destination
---
src/backend/utils/error/elog.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..dccbabf40a 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2228,8 +2228,22 @@ check_log_destination(char **newval, void **extra, GucSource source)
#endif
else
{
+ StringInfoData errhint;
+
+ initStringInfo();
+ appendStringInfoString(, "stderr");
+#ifdef HAVE_SYSLOG
+ appendStringInfoString(, ", syslog");
+#endif
+#ifdef WIN32
+ appendStringInfoString(, ", eventlog");
+#endif
+ appendStringInfoString(, ", csvlog, and jsonlog");
+
GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+ GUC_check_errhint("Valid values are combinations of %s.", errhint.data);
pfree(rawstring);
+ pfree(errhint.data);
list_free(elemlist);
return false;
}
--
2.41.0
Re: Add hint message for check_log_destination()
On Mon, 10 Jul 2023 at 14:23, Masahiko Sawada wrote:
> On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
> wrote:
>>
>> At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li wrote in
>> >
>> > On Sat, 08 Jul 2023 at 12:48, Michael Paquier wrote:
>> > > On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
>> > >> + appendStringInfoString(, "\"stderr\"");
>> > >> +#ifdef HAVE_SYSLOG
>> > >> + appendStringInfoString(, ", \"syslog\"");
>> > >> +#endif
>> > >> +#ifdef WIN32
>> > >> + appendStringInfoString(, ", \"eventlog\"");
>> > >> +#endif
>> > >> + appendStringInfoString(, ", \"csvlog\", and
>> > >> \"jsonlog\"");
>> > >
>> > > Hmm. Is that OK as a translatable string?
>
> It seems okay to me but needs to be checked.
>
>> >
>> >
>> > Sorry for the late reply! I'm not sure. How can I know whether it is
>> > translatable?
>>
>> At the very least, we can't generate comma-separated lists
>> programatically because punctuation marks vary across languages.
>>
>> One potential approach could involve defining the message for every
>> potential combination, in full length.
>
> Don't we generate a comma-separated list for an error hint of an enum
> parameter? For example, to generate the following error hint:
>
> =# alter system set client_min_messages = 'aaa';
> ERROR: invalid value for parameter "client_min_messages": "aaa"
> HINT: Available values: debug5, debug4, debug3, debug2, debug1, log,
> notice, warning, error.
>
> we use the comma-separated generated by config_enum_get_options() and
> do ereport() like:
>
> ereport(elevel,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>errmsg("invalid value for parameter \"%s\": \"%s\"",
> name, value),
>hintmsg ? errhint("%s", _(hintmsg)) : 0));
> IMO log_destination is a string GUC parameter but its value is the
> list of enums. So it makes sense to me to add a hint message like what
> we do for enum parameters in case where the user mistypes a wrong
> value. I'm not sure why the proposed patch needs to quote the usable
> values, though.
I borrowed the description from pg_settings extra_desc. In my first patch,
I used the hint message line enum parameter, however, since it might be a
combination of multiple log destinations, so, I update the patch using
extra_desc.
--
Regrads,
Japin Li.
Re: Add hint message for check_log_destination()
On Sat, 08 Jul 2023 at 12:48, Michael Paquier wrote: > On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote: >> +appendStringInfoString(, "\"stderr\""); >> +#ifdef HAVE_SYSLOG >> +appendStringInfoString(, ", \"syslog\""); >> +#endif >> +#ifdef WIN32 >> +appendStringInfoString(, ", \"eventlog\""); >> +#endif >> +appendStringInfoString(, ", \"csvlog\", and >> \"jsonlog\""); > > Hmm. Is that OK as a translatable string? Sorry for the late reply! I'm not sure. How can I know whether it is translatable? -- Regrads, Japin Li.
Re: Add hint message for check_log_destination()
On Fri, 07 Jul 2023 at 16:21, Masahiko Sawada wrote:
> On Fri, Jul 7, 2023 at 4:53 PM Japin Li wrote:
>>
>>
>> On Fri, 07 Jul 2023 at 14:46, jian he wrote:
>> > On Fri, Jul 7, 2023 at 1:06 PM Japin Li wrote:
>> >>
>> >>
>> >> Hi, hackers
>> >>
>> >> When I try to change log_destination using ALTER SYSTEM with the wrong
>> >> value,
>> >> it complains of the "Unrecognized key word" without available values.
>> >> This
>> >> patch tries to add a hint message that provides available values for
>> >> log_destination. Any thoughts?
>
> +1
>
> + appendStringInfo(, "\"stderr\"");
> +#ifdef HAVE_SYSLOG
> + appendStringInfo(, ", \"syslog\"");
> +#endif
> +#ifdef WIN32
> + appendStringInfo(, ", \"eventlog\"");
> +#endif
> + appendStringInfo(, ", \"csvlog\", and \"jsonlog\"");
>
> I think using appendStringInfoString() is a bit more natural and faster.
>
Thanks for your review! Fixed as per your suggession.
>From e0338c5085e655f9a25f13cd5acea9a3110806fd Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Fri, 7 Jul 2023 15:48:53 +0800
Subject: [PATCH v3 1/1] Add hint message for check_log_destination
---
src/backend/utils/error/elog.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..a7545dcbd9 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2228,8 +2228,22 @@ check_log_destination(char **newval, void **extra, GucSource source)
#endif
else
{
+ StringInfoData errhint;
+
+ initStringInfo();
+ appendStringInfoString(, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+ appendStringInfoString(, ", \"syslog\"");
+#endif
+#ifdef WIN32
+ appendStringInfoString(, ", \"eventlog\"");
+#endif
+ appendStringInfoString(, ", \"csvlog\", and \"jsonlog\"");
+
GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+ GUC_check_errhint("Valid values are combinations of %s.", errhint.data);
pfree(rawstring);
+ pfree(errhint.data);
list_free(elemlist);
return false;
}
--
2.25.1
--
Regrads,
Japin Li.
Re: Add hint message for check_log_destination()
On Fri, 07 Jul 2023 at 14:46, jian he wrote:
> On Fri, Jul 7, 2023 at 1:06 PM Japin Li wrote:
>>
>>
>> Hi, hackers
>>
>> When I try to change log_destination using ALTER SYSTEM with the wrong value,
>> it complains of the "Unrecognized key word" without available values. This
>> patch tries to add a hint message that provides available values for
>> log_destination. Any thoughts?
>>
>
> select * from pg_settings where name ~* 'log.*destin*' \gx
>
> short_desc | Sets the destination for server log output.
> extra_desc | Valid values are combinations of "stderr", "syslog",
> "csvlog", "jsonlog", and "eventlog", depending on the platform.
>
> you can just reuse extra_desc in the pg_settings (view) column ?
Thanks for your review!
Yeah, the description of extra_desc is more accurate. Updated.
--
Regrads,
Japin Li.
>From 715f9811717c9d27f6b2f41db639b18e6ba58625 Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Fri, 7 Jul 2023 15:48:53 +0800
Subject: [PATCH v2 1/1] Add hint message for check_log_destination
---
src/backend/utils/error/elog.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..a32f9613be 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2228,8 +2228,22 @@ check_log_destination(char **newval, void **extra, GucSource source)
#endif
else
{
+ StringInfoData errhint;
+
+ initStringInfo();
+ appendStringInfo(, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+ appendStringInfo(, ", \"syslog\"");
+#endif
+#ifdef WIN32
+ appendStringInfo(, ", \"eventlog\"");
+#endif
+ appendStringInfo(, ", \"csvlog\", and \"jsonlog\"");
+
GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+ GUC_check_errhint("Valid values are combinations of %s.", errhint.data);
pfree(rawstring);
+ pfree(errhint.data);
list_free(elemlist);
return false;
}
--
2.25.1
Add hint message for check_log_destination()
Hi, hackers
When I try to change log_destination using ALTER SYSTEM with the wrong value,
it complains of the "Unrecognized key word" without available values. This
patch tries to add a hint message that provides available values for
log_destination. Any thoughts?
--
Regrads,
Japin Li.
>From 7d6b50c9deaba576cdc28e170bff541f630415f9 Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Fri, 7 Jul 2023 12:59:09 +0800
Subject: [PATCH v1 1/1] Add hint message for log_destination
---
src/backend/utils/error/elog.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..fdc6557a59 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2228,8 +2228,21 @@ check_log_destination(char **newval, void **extra, GucSource source)
#endif
else
{
+ StringInfoData errhint;
+
+ initStringInfo();
+ appendStringInfo(, "stderr, csvlog, jsonlog");
+#ifdef HAVE_SYSLOG
+ appendStringInfo(, ", syslog");
+#endif
+#ifdef WIN32
+ appendStringInfo(, ", eventlog");
+#endif
+
GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+ GUC_check_errhint("Available values: %s.", errhint.data);
pfree(rawstring);
+ pfree(errhint.data);
list_free(elemlist);
return false;
}
--
2.25.1
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
On Tue, 04 Jul 2023 at 17:00, jian he wrote:
> the following will also crash. no idea why.
> begin;
> select count(*) from onek;
> select relpages from pg_class where relname = 'onek'; --queryA
>
> SELECT count(*) FROM pg_buffercache WHERE relfilenode =
> pg_relation_filenode('onek'::regclass); --queryB
>
> insert into onek values(default);
>
> select count(pg_buffercache_invalidate(bufferid)) from
> pg_buffercache where relfilenode =
> pg_relation_filenode('onek'::regclass);
>
> -
> queryA returns 35, queryB returns 37.
> --
> crash info:
> test_dev=*# insert into onek values(default);
> INSERT 0 1
> test_dev=*# select count(pg_buffercache_invalidate(bufferid)) from
> pg_buffercache where relfilenode =
> pg_relation_filenode('onek'::regclass);
> TRAP: failed Assert("resarr->nitems < resarr->maxitems"), File:
> "../../Desktop/pg_sources/main/postgres/src/backend/utils/resowner/resowner.c",
> Line: 275, PID: 1533312
According to the comments of ResourceArrayAdd(), the caller must have previously
done ResourceArrayEnlarge(). I tried to call ResourceOwnerEnlargeBuffers()
before
PinBuffer_Locked(), so it can avoid this crash.
if ((buf_state & BM_DIRTY) == BM_DIRTY)
{
+ /* make sure we can handle the pin */
+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+
/*
* Try once to flush the dirty buffer.
*/
PinBuffer_Locked(bufHdr);
--
Regrads,
Japin Li.
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
On Mon, 03 Jul 2023 at 16:26, Palak Chaturvedi
wrote:
> Hi Thomas,
> Thank you for your suggestions. I have added the sql in the meson
> build as well.
>
> On Sat, 1 Jul 2023 at 03:39, Thomas Munro wrote:
>>
>> On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi
>> wrote:
>> > pgbench=# select count(pg_buffercache_invalidate(bufferid)) from
>> > pg_buffercache where relfilenode =
>> > pg_relation_filenode('pgbench_accounts'::regclass);
>>
>> Hi Palak,
>>
>> Thanks for working on this! I think this will be very useful for
>> testing existing workloads but also for testing future work on
>> prefetching with AIO (and DIO), work on putting SLRUs (or anything
>> else) into the buffer pool, nearby proposals for caching buffer
>> mapping information, etc etc.
>>
>> Palak and I talked about this idea a bit last week (stimulated by a
>> recent thread[1], but the topic has certainly come up before), and we
>> discussed some different ways one could specify which pages are
>> dropped. For example, perhaps the pg_prewarm extension could have an
>> 'unwarm' option instead. I personally thought the buffer ID-based
>> approach was quite good because it's extremely simple, while giving
>> the user the full power of SQL to say which buffers. Half a table?
>> Visibility map? Everything? Root page of an index? I think that's
>> probably better than something that requires more code and
>> complication but is less flexible in the end. It feels like the right
>> level of rawness for something primarily of interest to hackers and
>> advanced users. I don't think it matters that there is a window
>> between selecting a buffer ID and invalidating it, for the intended
>> use cases. That's my vote, anyway, let's see if others have other
>> ideas...
>>
>> We also talked a bit about how one might control the kernel page cache
>> in more fine-grained ways for testing purposes, but it seems like the
>> pgfincore project has that covered with its pgfadvise_willneed() and
>> pgfadvise_dontneed(). IMHO that project could use more page-oriented
>> operations (instead of just counts and coarse grains operations) but
>> that's something that could be material for patches to send to the
>> extension maintainers. This work, in contrast, is more tangled up
>> with bufmgr.c internals, so it feels like this feature belongs in a
>> core contrib module.
>>
>> Some initial thoughts on the patch:
>>
>> I wonder if we should include a simple exercise in
>> contrib/pg_buffercache/sql/pg_buffercache.sql. One problem is that
>> it's not guaranteed to succeed in general. It doesn't wait for pins
>> to go away, and it doesn't retry cleaning dirty buffers after one
>> attempt, it just returns false, which I think is probably the right
>> approach, but it makes the behaviour too non-deterministic for simple
>> tests. Perhaps it's enough to include an exercise where we call it a
>> few times to hit a couple of cases, but not verify what effect it has.
>>
>> It should be restricted by role, but I wonder which role it should be.
>> Testing for superuser is now out of fashion.
>>
>> Where the Makefile mentions 1.4--1.5.sql, the meson.build file needs
>> to do the same. That's because PostgreSQL is currently in transition
>> from autoconf/gmake to meson/ninja[2], so for now we have to maintain
>> both build systems. That's why it fails to build in some CI tasks[3].
>> You can enable CI in your own GitHub account if you want to run test
>> builds on several operating systems, see [4] for info.
>>
>> [1]
>> https://www.postgresql.org/message-id/flat/CAFSGpE3y_oMK1uHhcHxGxBxs%2BKrjMMdGrE%2B6HHOu0vttVET0UQ%40mail.gmail.com
>> [2] https://wiki.postgresql.org/wiki/Meson
>> [3] http://cfbot.cputube.org/palak-chaturvedi.html
>> [4]
>> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/ci/README;hb=HEAD
I think, zero is not a valid buffer identifier. See src/include/storage/buf.h.
+ bufnum = PG_GETARG_INT32(0);
+ if (bufnum < 0 || bufnum > NBuffers)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("buffernum is not valid")));
+
+ }
If we use SELECT pg_buffercache_invalidate(0), it will crash.
--
Regrads,
Japin Li.
Re: Another incorrect comment for pg_stat_statements
On Thu, 29 Jun 2023 at 08:19, Michael Paquier wrote: > On Wed, Jun 28, 2023 at 09:26:02PM +0800, Japin Li wrote: >> +1. LGTM. > > Nothing much to add, so applied with the initial comment fix. Thanks! -- Regrads, Japin Li.
Re: Another incorrect comment for pg_stat_statements
On Wed, 28 Jun 2023 at 16:27, Richard Guo wrote: > On Wed, Jun 28, 2023 at 3:36 PM Michael Paquier wrote: > >> On Wed, Jun 28, 2023 at 03:09:55PM +0800, Richard Guo wrote: >> > +1. To nitpick, how about we remove the blank line just before removing >> > the key for top level entry? >> > >> > - /* Also remove entries for top level statements */ >> > + /* Also remove entries if exist for top level statements */ >> >key.toplevel = true; >> > - >> > - /* Remove the key if exists */ >> >entry = (pgssEntry *) hash_search(pgss_hash, , HASH_REMOVE, NULL); >> >> Why not if it improves the overall situation. Could you send a patch >> with everything you have in mind? > > > Here is the patch. I don't have too much in mind, so the patch just > removes the blank line and revises the comment a bit. > +1. LGTM. -- Regrads, Japin Li.
Re: Another incorrect comment for pg_stat_statements
On Wed, 28 Jun 2023 at 11:22, Richard Guo wrote: > On Wed, Jun 28, 2023 at 10:53 AM Japin Li wrote: > >> >> Hi, hackers >> >> There has $subject that introduced by commit 6b4d23feef6. When we reset >> the entries >> if all parameters are avaiable, non-top-level entries removed first, then >> top-level >> entries. > > > I did not see the diffs. Maybe uploaded the wrong attachment? > My bad! Here is the patch. Thanks! diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 21bede29fe..670c993816 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -2552,7 +2552,7 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid) key.dbid = dbid; key.queryid = queryid; - /* Remove the key if it exists, starting with the top-level entry */ + /* Remove the key if it exists, starting with the non-top-level entry */ key.toplevel = false; entry = (pgssEntry *) hash_search(pgss_hash, , HASH_REMOVE, NULL); if (entry)/* found */ -- Regrads, Japin Li.
Another incorrect comment for pg_stat_statements
Hi, hackers There has $subject that introduced by commit 6b4d23feef6. When we reset the entries if all parameters are avaiable, non-top-level entries removed first, then top-level entries. On branch REL_15_STABLE Your branch is up to date with 'origin/REL_15_STABLE'. Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git restore ..." to discard changes in working directory) modified: contrib/pg_stat_statements/pg_stat_statements.c no changes added to commit (use "git add" and/or "git commit -a") -- Regrads, Japin Li.
Incorrect comment for memset() on pgssHashKey?
Hi, Commit 6b4d23feef introduces a toplevel field in pgssHashKey, which leads padding. In pgss_store(), it comments that memset() is required when pgssHashKey is without padding only. @@ -1224,9 +1227,14 @@ pgss_store(const char *query, uint64 queryId, query = CleanQuerytext(query, _location, _len); /* Set up key for hashtable search */ + +/* memset() is required when pgssHashKey is without padding only */ +memset(, 0, sizeof(pgssHashKey)); + key.userid = GetUserId(); key.dbid = MyDatabaseId; key.queryid = queryId; +key.toplevel = (exec_nested_level == 0); /* Lookup the hash table entry with shared lock. */ LWLockAcquire(pgss->lock, LW_SHARED); However, we need memset() only when pgssHashKey has padding, right? -- Regrads, Japin Li. diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 21bede29fe..f6ed87f1e6 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1257,7 +1257,7 @@ pgss_store(const char *query, uint64 queryId, /* Set up key for hashtable search */ - /* memset() is required when pgssHashKey is without padding only */ + /* memset() is required when pgssHashKey is with padding only */ memset(, 0, sizeof(pgssHashKey)); key.userid = GetUserId();
Re: Replace (GUC_UNIT_MEMORY | GUC_UNIT_TIME) with GUC_UNIT in guc.c
On Wed, 14 Jun 2023 at 17:52, Richard Guo wrote: > On Wed, Jun 14, 2023 at 3:47 PM Michael Paquier wrote: > >> On Wed, Jun 14, 2023 at 03:38:10PM +0800, Richard Guo wrote: >> > +1. BTW, it seems that GUC_UNIT_TIME is not used anywhere except in >> > GUC_UNIT. I was wondering if we can retire it, but maybe we'd better >> > not. It still indicates that we need to use time units table. >> >> Some out-of-core code declaring custom GUCs could rely on that, so >> it is better not to remove it. > > > I see. Thanks for pointing that out. > Thanks for all of your reviews. Agreed with Michael do not touch GUC_UNIT_TIME. -- Regrads, Japin Li.
Replace (GUC_UNIT_MEMORY | GUC_UNIT_TIME) with GUC_UNIT in guc.c
Hi, hackers
We use (GUC_UNIT_MEMORY | GUC_UNIT_TIME) instead of GUC_UNIT even though we
already define it in guc.h.
Maybe using GUC_UNIT is better? Here is a patch to fix it.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a9033b7a54..5308896c87 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2766,7 +2766,7 @@ convert_real_from_base_unit(double base_value, int
base_unit,
const char *
get_config_unit_name(int flags)
{
-switch (flags & (GUC_UNIT_MEMORY | GUC_UNIT_TIME))
+switch (flags & GUC_UNIT)
{
case 0:
return NULL;/* GUC has no units */
@@ -2802,7 +2802,7 @@ get_config_unit_name(int flags)
return "min";
default:
elog(ERROR, "unrecognized GUC units value: %d",
- flags & (GUC_UNIT_MEMORY | GUC_UNIT_TIME));
+ flags & GUC_UNIT);
return NULL;
}
}
--
Regrads,
Japin Li.
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)
Unwanted file mode modification?
Hi, Commit 216a784829 change the src/backend/replication/logical/worker.c file mode from 0644 to 0755, which is unwanted, right? [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=216a784829c2c5f03ab0c43e009126cbb819e9b2 -- 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.
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.
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: redundant check of msg in does_not_exist_skipping
> On Nov 18, 2022, at 4:04 AM, Robert Haas wrote: > > On Thu, Nov 17, 2022 at 10:56 AM Tom Lane wrote: >> This is a completely bad idea. If it takes that level of analysis >> to see that msg can't be null, we should leave the test in place. >> Any future modification of either this code or what it calls could >> break the conclusion. > > +1. Also, even if the check were quite obviously useless, it's cheap > insurance. It's difficult to believe that it hurts performance in any > measurable way. If anything, we would benefit from having more sanity > checks in our code, rather than fewer. > Thanks for the explanation! Got it.
Re: redundant check of msg in does_not_exist_skipping
On Thu, 17 Nov 2022 at 23:06, Japin Li wrote:
> On Thu, 17 Nov 2022 at 20:12, Ted Yu wrote:
>> Hi,
>> I was looking at commit aca992040951c7665f1701cd25d48808eda7a809
>>
>> I think the check of msg after the switch statement is not necessary. The
>> variable msg is used afterward.
>> If there is (potential) missing case in switch statement, the compiler
>> would warn.
>>
>> How about removing the check ?
>>
>
> I think we cannot remove the check, for example, if objtype is
> OBJECT_OPFAMILY,
> and schema_does_not_exist_skipping() returns true, the so the msg keeps NULL,
> if we remove this check, a sigfault might be occurd in ereport().
>
> case OBJECT_OPFAMILY:
> {
> List *opfname = list_copy_tail(castNode(List, object),
> 1);
>
> if (!schema_does_not_exist_skipping(opfname, , ))
> {
> msg = gettext_noop("operator family \"%s\" does not exist
> for access method \"%s\", skipping");
> name = NameListToString(opfname);
> args = strVal(linitial(castNode(List, object)));
> }
> }
> break;
Sorry, I didn't look into schema_does_not_exist_skipping(), and after look
into schema_does_not_exist_skipping function and others, all paths that go
out switch branch has non-NULL for msg, so we can remove this check safely.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: redundant check of msg in does_not_exist_skipping
On Thu, 17 Nov 2022 at 20:12, Ted Yu wrote:
> Hi,
> I was looking at commit aca992040951c7665f1701cd25d48808eda7a809
>
> I think the check of msg after the switch statement is not necessary. The
> variable msg is used afterward.
> If there is (potential) missing case in switch statement, the compiler
> would warn.
>
> How about removing the check ?
>
I think we cannot remove the check, for example, if objtype is OBJECT_OPFAMILY,
and schema_does_not_exist_skipping() returns true, the so the msg keeps NULL,
if we remove this check, a sigfault might be occurd in ereport().
case OBJECT_OPFAMILY:
{
List *opfname = list_copy_tail(castNode(List, object), 1);
if (!schema_does_not_exist_skipping(opfname, , ))
{
msg = gettext_noop("operator family \"%s\" does not exist
for access method \"%s\", skipping");
name = NameListToString(opfname);
args = strVal(linitial(castNode(List, object)));
}
}
break;
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: Typo for xl_running_xacts
On Thu, 17 Nov 2022 at 16:22, Daniel Gustafsson wrote: >> On 17 Nov 2022, at 08:06, Japin Li wrote: > >> I find some typo about xl_running_xacts in comments. >> Attached a patch to fix those. > > Thanks, applied! > >> - * might look that we could use xl_running_xact's ->xids information to >> + * might look that we could use xl_running_xacts->xids information to > > I'm not a native english speaker, but since xl_running_xacts refers to a name > in this case and not an indication of plural possession, ending with a 's is > correct even if it ends with an 's', so I instead changed this to > "xl_running_xacts's". Thanks, I found other places use "xl_running_xacs'", such as "xl_running_xacts' oldestRunningXid" in snapbuild.c. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Typo for xl_running_xacts
Hi, hackers I find some typo about xl_running_xacts in comments. Attached a patch to fix those. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd. diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index 5006a5c464..44d94bf656 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -93,7 +93,7 @@ * Initially the machinery is in the START stage. When an xl_running_xacts * record is read that is sufficiently new (above the safe xmin horizon), * there's a state transition. If there were no running xacts when the - * running_xacts record was generated, we'll directly go into CONSISTENT + * xl_running_xacts record was generated, we'll directly go into CONSISTENT * state, otherwise we'll switch to the BUILDING_SNAPSHOT state. Having a full * snapshot means that all transactions that start henceforth can be decoded * in their entirety, but transactions that started previously can't. In @@ -1331,7 +1331,7 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn */ /* - * xl_running_xact record is older than what we can use, we might not have + * xl_running_xacts record is older than what we can use, we might not have * all necessary catalog rows anymore. */ if (TransactionIdIsNormal(builder->initial_xmin_horizon) && @@ -1399,7 +1399,7 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn * encountered. In that case, switch to BUILDING_SNAPSHOT state, and * record xl_running_xacts->nextXid. Once all running xacts have finished * (i.e. they're all >= nextXid), we have a complete catalog snapshot. It - * might look that we could use xl_running_xact's ->xids information to + * might look that we could use xl_running_xacts->xids information to * get there quicker, but that is problematic because transactions marked * as running, might already have inserted their commit record - it's * infeasible to change that with locking.
Re: Memory leak in adjust_data_dir
On Wed, 16 Nov 2022 at 12:19, Tom Lane wrote:
> Japin Li writes:
>> Hi, hackers,
>
> ITYM pgsql-hackers, this is off-topic here.
>
Sorry for typo the email address.
>> When I'm reviewing patch [1], I find there is a memory leak in
>> adjust_data_dir(), the cmd was allocated by psprintf(), but forget
>> releasing.
>
> Can't get excited about it in pg_ctl; that program won't run
> long enough for anybody to notice.
>
Yeah, it won't run a long time. I find that the memory of my_exec_path
was released, so I think we also should do release on cmd. IMO, this is
a bit confused when should we do release the memory of variables for
short lifetime?
[Here is the origin contents which I send a wrong mail-list]
Hi, hackers,
When I'm reviewing patch [1], I find there is a memory leak in
adjust_data_dir(), the cmd was allocated by psprintf(), but forget
releasing.
[1]
https://www.postgresql.org/message-id/CALte62y3yZpHNFnYVz1uACaFbmb6go9fyeRaO5uHF5XaxtarbA%40mail.gmail.com
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index ceab603c47..ace2d676fc 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2159,6 +2159,7 @@ adjust_data_dir(void)
write_stderr(_("%s: could not determine the data directory
using command \"%s\"\n"), progname, cmd);
exit(1);
}
+ free(cmd);
free(my_exec_path);
/* strip trailing newline and carriage return */
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: closing file in adjust_data_dir
On Wed, 16 Nov 2022 at 11:15, Ted Yu wrote: > On Tue, Nov 15, 2022 at 7:12 PM Japin Li wrote: >> After some rethinking, I find the origin code do not have problems. >> >> If fd is NULL or fgets() returns NULL, the process exits. Otherwise, we >> call >> pclose() to close fd. The code isn't straightforward, however, it is >> correct. >> >> >> >> Please read this sentence from my first post: > > If the fgets() call doesn't return NULL, the pclose() would be skipped. fgets() returns non-NULL, it means the second condition is false, and it will check the third condition, which calls pclose(), so it cannot be skipped, right? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: closing file in adjust_data_dir
On Wed, 16 Nov 2022 at 10:52, Ted Yu wrote:
> On Tue, Nov 15, 2022 at 6:35 PM Japin Li wrote:
>>
>> fd = popen(cmd, "r");
>> - if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL ||
>> pclose(fd) != 0)
>> + if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
>> {
>> + pclose(fd);
>> write_stderr(_("%s: could not determine the data directory
>> using command \"%s\"\n"), progname, cmd);
>> exit(1);
>> }
>>
>> Here, segfault maybe occurs if fd is NULL. I think we can remove pclose()
>> safely since the process will exit.
>>
>
> That means we're going back to v1 of the patch.
>
After some rethinking, I find the origin code do not have problems.
If fd is NULL or fgets() returns NULL, the process exits. Otherwise, we call
pclose() to close fd. The code isn't straightforward, however, it is correct.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
