Re: Proposal: remove obsolete hot-standby testing infrastructure
Hello Tom, 04.01.2022 00:50, Tom Lane wrote: > The attached proposed patch removes some ancient infrastructure for > manually testing hot standby. I doubt anyone has used this in years, > because AFAICS there is nothing here that's not done better by the > src/test/recovery TAP tests. (Or if there is, we ought to migrate > it into the TAP tests.) > > Thoughts? It's hardly that important, but we (Postgres Pro) run this test regularly to check for primary-standby compatibility. It's useful when checking binary packages from different minor versions. For example, we setup postgresql-14.0 and postgresql-14.1 aside (renaming one installation' directory and changing it's port) and perform the test. What've found with it was e.g. incompatibility due to linkage of different libicu versions (that was PgPro-only issue). I don't remember whether we found something related to PostgreSQL itself, but we definitely use this test and I'm not sure how to replace it in our setup with a TAP test. On the other hand, testing binaries is not accustomed in the community yet, so when such testing will be adopted, probably a brand new set of tests should emerge. Best regards, Alexander
Re: PostgreSQL stops when adding a breakpoint in CLion
> In a standalone backend, I think there are only 3 ways to get to
> normal shutdown:
>* SIGTERM
>* SIGQUIT
> * EOF on stdin
I debugged a bit more and I see that getc() returns with -1 in
interactive_getc() which is interpreted as EOF:
c = getc(stdin);
I see that errno == EINTR when it happens. This is as much as I can figure
out in C, so I'm leaving it at that. Your advice about debugging the
backend process ("select pg_backend_pid()") instead of running in a
single-user mode worked for me, thank you!
On Tue, Jan 4, 2022 at 1:02 AM Tom Lane wrote:
> Stanislav Bashkyrtsev writes:
> >> Why do you think postgres quits?
>
> > The process was running and then it stopped. And in the console I see:
> > 2022-01-03 23:23:29.495 MSK [76717] LOG: checkpoint starting: shutdown
> > immediate
>
> In a standalone backend, I think there are only 3 ways to get to
> normal shutdown:
> * SIGTERM
> * SIGQUIT
> * EOF on stdin
>
> It's not very clear which of those your setup is triggering.
>
> In any case, debugging standalone mode is very very rarely
> what you should be doing; it's only vaguely related to normal
> operation, plus you lack all the creature comforts of psql.
> The usual thing is to start a normal interactive session, find out
> the PID of its connected backend process ("select pg_backend_pid()"
> is a reliable way), and then attach to that process with GDB or your
> debugger of choice.
>
> regards, tom lane
>
Re: SQL/JSON: functions
On Thu, Dec 16, 2021 at 3:06 AM Andrew Dunstan wrote:
>
>
> On 12/9/21 09:04, Himanshu Upadhyaya wrote:
> >
> >
> >
> > 4)
> > Are we intentionally allowing numeric keys in JSON_OBJECT but somehow
> > these are not allowed in ORACLE?
> > ‘postgres[151876]=#’select JSON_OBJECT( 3+1:2, 2+2:1);
> > json_object
> >
> > {"4" : 2, "4" : 1}
> > (1 row)
> >
> > In ORACLE we are getting error("ORA-00932: inconsistent datatypes:
> > expected CHAR got NUMBER") which seems to be more reasonable.
> > "ORA-00932: inconsistent datatypes: expected CHAR got NUMBER"
> >
> > Postgres is also dis-allowing below then why allow numeric keys in
> > JSON_OBJECT?
> > ‘postgres[151876]=#’select '{
> > "track": {
> > "segments": [
> > {
> > "location": [ 47.763, 13.4034 ],
> > "start time": "2018-10-14 10:05:14",
> > "HR": 73
> > },
> > {
> > "location": [ 47.706, 13.2635 ],
> > "start time": "2018-10-14 10:39:21",
> > 3: 135
> > }
> > ]
> > }
> > }'::jsonb;
> > ERROR: 22P02: invalid input syntax for type json
> > LINE 1: select '{
> >^
> > DETAIL: Expected string, but found "3".
> > CONTEXT: JSON data, line 12: 3...
> > LOCATION: json_ereport_error, jsonfuncs.c:621
> >
> > Also, JSON_OBJECTAGG is failing if we have any numeric key, however,
> > the message is not very appropriate.
> > SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
> > FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
> > (5,5)) kv(k, v);
> > ERROR: 22P02: invalid input syntax for type integer: "no"
> > LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
> > ^
> > LOCATION: pg_strtoint32, numutils.c:320
> >
> >
> >
>
> The literal above is simply not legal json, so the json parser is going
> to reject it outright. However it is quite reasonable for JSON
> constructors to convert non-string key values to strings. Otherwise we'd
> be rejecting not just numbers but for example dates as key values. c.f.
> json_build_object(), the documentation for which says "Key arguments are
> coerced to text."
>
Yes Agree on this, but just thinking if we can differentiate dates and
numeric keys to have consistent behaviour and simply reject if we have
numeric keys(to match it with the behaviour of JSON parser) because
JSON with numeric keys is actually not a valid JSON.
SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
(5,5)) kv(k, v);
ERROR: 22P02: invalid input syntax for type integer: "no"
LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
^
LOCATION: pg_strtoint32, numutils.c:320
Above call to JSON_OBJECTAGG is failing because we have the numeric
key, is not that it also needs to follow the same context of
converting key argument to text? or both(JSON_OBJECTAGG and
JSON_OBJECT) should not allow numeric keys in the JSON object and
allow date (if that is the only use case)?
Thoughts?
--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
Re: SQL/JSON: functions
út 4. 1. 2022 v 10:19 odesílatel Himanshu Upadhyaya <
upadhyaya.himan...@gmail.com> napsal:
> On Thu, Dec 16, 2021 at 3:06 AM Andrew Dunstan
> wrote:
> >
> >
> > On 12/9/21 09:04, Himanshu Upadhyaya wrote:
> > >
> > >
> > >
> > > 4)
> > > Are we intentionally allowing numeric keys in JSON_OBJECT but somehow
> > > these are not allowed in ORACLE?
> > > ‘postgres[151876]=#’select JSON_OBJECT( 3+1:2, 2+2:1);
> > > json_object
> > >
> > > {"4" : 2, "4" : 1}
> > > (1 row)
> > >
> > > In ORACLE we are getting error("ORA-00932: inconsistent datatypes:
> > > expected CHAR got NUMBER") which seems to be more reasonable.
> > > "ORA-00932: inconsistent datatypes: expected CHAR got NUMBER"
> > >
> > > Postgres is also dis-allowing below then why allow numeric keys in
> > > JSON_OBJECT?
> > > ‘postgres[151876]=#’select '{
> > > "track": {
> > > "segments": [
> > > {
> > > "location": [ 47.763, 13.4034 ],
> > > "start time": "2018-10-14 10:05:14",
> > > "HR": 73
> > > },
> > > {
> > > "location": [ 47.706, 13.2635 ],
> > > "start time": "2018-10-14 10:39:21",
> > > 3: 135
> > > }
> > > ]
> > > }
> > > }'::jsonb;
> > > ERROR: 22P02: invalid input syntax for type json
> > > LINE 1: select '{
> > >^
> > > DETAIL: Expected string, but found "3".
> > > CONTEXT: JSON data, line 12: 3...
> > > LOCATION: json_ereport_error, jsonfuncs.c:621
> > >
> > > Also, JSON_OBJECTAGG is failing if we have any numeric key, however,
> > > the message is not very appropriate.
> > > SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
> > > FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
> > > (5,5)) kv(k, v);
> > > ERROR: 22P02: invalid input syntax for type integer: "no"
> > > LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
> > > ^
> > > LOCATION: pg_strtoint32, numutils.c:320
> > >
> > >
> > >
> >
> > The literal above is simply not legal json, so the json parser is going
> > to reject it outright. However it is quite reasonable for JSON
> > constructors to convert non-string key values to strings. Otherwise we'd
> > be rejecting not just numbers but for example dates as key values. c.f.
> > json_build_object(), the documentation for which says "Key arguments are
> > coerced to text."
> >
> Yes Agree on this, but just thinking if we can differentiate dates and
> numeric keys to have consistent behaviour and simply reject if we have
> numeric keys(to match it with the behaviour of JSON parser) because
> JSON with numeric keys is actually not a valid JSON.
>
> +1
Pavel
> SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
> FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
> (5,5)) kv(k, v);
> ERROR: 22P02: invalid input syntax for type integer: "no"
> LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
> ^
> LOCATION: pg_strtoint32, numutils.c:320
>
> Above call to JSON_OBJECTAGG is failing because we have the numeric
> key, is not that it also needs to follow the same context of
> converting key argument to text? or both(JSON_OBJECTAGG and
> JSON_OBJECT) should not allow numeric keys in the JSON object and
> allow date (if that is the only use case)?
>
> Thoughts?
> --
> Regards,
> Himanshu Upadhyaya
> EnterpriseDB: http://www.enterprisedb.com
>
>
>
Re: Clarify planner_hook calling convention
On 1/3/22 8:59 PM, Tom Lane wrote: "Andrey V. Lepikhov" writes: planner hook is frequently used in monitoring and advising extensions. Yeah. The call to this hook is implemented in the way, that the standard_planner routine must be called at least once in the hook's call chain. But, as I see in [1], it should allow us "... replace the planner altogether". In such situation it haven't sense to call standard_planner at all. That's possible in theory, but who's going to do it in practice? We use it in an extension that freezes a plan for specific parameterized query (using plancache + shared storage) - exactly the same technique as extended query protocol does, but spreading across all backends. As I know, the community doesn't like such features, and we use it in enterprise code only. But, maybe more simple solution is to describe requirements to such kind of extensions in the code and documentation (See patch in attachment)? + * 2. If your extension implements some planning activity, write in the extension + * docs a requirement to set the extension at the begining of shared libraries list. This advice seems pretty unhelpful. If more than one extension is getting into the planner_hook, they can't all be first. I want to check planner_hook on startup and log an error if it isn't NULL and give a user an advice how to fix it. I want to legalize this logic, if permissible. (Also, largely the same issue applies to very many of our other hooks.) Agreed. Interference between extensions is a very annoying issue now. -- regards, Andrey Lepikhov Postgres Professional
Re: [PATCH] pg_stat_toast v6
Am 03.01.22 um 22:03 schrieb Justin Pryzby:
+pgstat_report_toast_activity(Oid relid, int attr,
+ bool externalized,
+ bool compressed,
+ int32 old_size,
+ int32 new_size,
...
+ if (new_size)
+ {
+ htabent->t_counts.t_size_orig+=old_size;
+ if (new_size)
+ {
I guess one of these is supposed to say old_size?
Didn't make a difference, tbth, as they'd both be 0 or have a value.
Streamlined the whole block now.
+CREATE TABLE toast_test (cola TEXT, colb TEXT COMPRESSION lz4, colc TEXT ,
cold TEXT, cole TEXT);
Is there a reason this uses lz4 ?
I thought it might help later on, but alas! the LZ4 column mainly broke
things, so I removed it for the time being.
If that's needed for stable results, I think you should use pglz, since that's
what's guaranteed to exist. I imagine LZ4 won't be required any time soon,
seeing as zlib has never been required.
Yeah. It didn't prove anything whatsoever.
+Be aware that this feature, depending on the amount of TOASTable
columns in
+your databases, may significantly increase the size of the statistics
files
+and the workload of the statistics collector. It is recommended to only
+temporarily activate this to assess the right compression and storage
method
+for (a) column(s).
saying "a column" is fine
Changed.
+ schemaname name
+ Attribute (column) number in the relation
+ relname name
+
+ compressmethod char
+
+
+ Compression method of the attribute (empty means default)
One thing to keep in mind is that the current compression method is only used
for *new* data - old data can still use the old compression method. It
probably doesn't need to be said here, but maybe you can refer to the docs
about that in alter_table.
+ Number of times the compression was successful (gained a size reduction)
It's more clear to say "was reduced in size"
Changed the wording a bit, I guess it is clear enough now.
The question is if the column should be there at all, as it's simply
fetched from pg_attribute...
+ /* we assume this inits to all zeroes: */
+ static const PgStat_ToastCounts all_zeroes;
You don't have to assume; static/global allocations are always zero unless
otherwise specified.
Copy-pasta ;-)
Removed.
Thx for looking into this!
Patch v7 will be in the next mail.
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bl...@pro-open.de
__
"Ceterum censeo SystemD esse delendam" - Cato
Re: [PATCH] pg_stat_toast v6
Am 03.01.22 um 22:23 schrieb Alvaro Herrera: Overall I think this is a good feature to have; assessing the need for compression is important for tuning, so +1 for the goal of the patch. Much appreciated! I didn't look into the patch carefully, but here are some minor comments: On 2022-Jan-03, Gunnar "Nick" Bluth wrote: @@ -229,7 +230,9 @@ toast_tuple_try_compression(ToastTupleContext *ttc, int attribute) Datum *value = &ttc->ttc_values[attribute]; Datum new_value; ToastAttrInfo *attr = &ttc->ttc_attr[attribute]; + instr_time start_time; + INSTR_TIME_SET_CURRENT(start_time); new_value = toast_compress_datum(*value, attr->tai_compression); if (DatumGetPointer(new_value) != NULL) Don't INSTR_TIME_SET_CURRENT unconditionally; in some systems it's an expensive syscall. Find a way to only do it if the feature is enabled. Yeah, I was worried about that (and asking if it would be required) already. Adding the check was easier than I expected, though I'm absolutely clueless if I did it right! #include "pgstat.h" extern PGDLLIMPORT bool pgstat_track_toast; This also suggests that perhaps it'd be a good idea to allow this to be enabled for specific tables only, rather than system-wide. (Maybe in order for stats to be collected, the user should have to both set the GUC option *and* set a per-table option? Not sure.) That would of course be nice, but I seriously doubt the required additional logic would be justified. The patch currently tampers with as few internal structures as possible, and for good reason... ;-) @@ -82,10 +82,12 @@ typedef enum StatMsgType PGSTAT_MTYPE_DEADLOCK, PGSTAT_MTYPE_CHECKSUMFAILURE, PGSTAT_MTYPE_REPLSLOT, + PGSTAT_MTYPE_CONNECTION, I think this new enum value doesn't belong in this patch. Yeah, did I mention I'm struggling with rebasing? ;-| +/* -- + * PgStat_ToastEntry Per-TOAST-column info in a MsgFuncstat + * -- Not in "a MsgFuncstat", right? Obviously... fixed! +-- function to wait for counters to advance +create function wait_for_stats() returns void as $$ I don't think we want a separate copy of wait_for_stats; see commit fe60b67250a3 and the discussion leading to it. Maybe you'll want to move the test to stats.sql. I'm not sure what to say about relying on Did so. LZ4; maybe you'll want to leave that part out, and just verify in an LZ4-enabled build that some 'l' entry exists. BTW, don't we have any decent way to turn that 'l' into a more reasonable, descriptive string? Also, perhaps make the view-defining query turn an empty compression method into whatever the default is. I'm not even sure that having it in there is useful at all. It's simply JOINed in from pg_attribute. Which is where I'd see that "make it look nicer" change happening, tbth. ;-) Or even better, stats collection should store the real compression method used rather than empty string, to avoid confusing things when some stats are collected, then the default is changed, then some more stats are collected. I was thinking about that already, but came to the conclusion that it a) would blow up the size of these statistics by quite a bit and b) would be quite tricky to display in a useful way. I mean, the use case of track_toast is pretty limited anyway; you'll probably turn this feature on with a specific column in mind, of which you'll probably know which compression method is used at the time. Thanks for the feedback! v7 attached. -- Gunnar "Nick" Bluth Eimermacherweg 106 D-48159 Münster Mobil +49 172 8853339 Email: gunnar.bl...@pro-open.de __ "Ceterum censeo SystemD esse delendam" - CatoFrom f07213d68c646ba64757e551e3587aab0ff221df Mon Sep 17 00:00:00 2001 From: "Gunnar \"Nick\" Bluth" Date: Tue, 4 Jan 2022 12:08:32 +0100 Subject: [PATCH v7] pg_stat_toast v7 --- doc/src/sgml/config.sgml | 26 ++ doc/src/sgml/monitoring.sgml | 163 ++ doc/src/sgml/storage.sgml | 12 +- src/backend/access/table/toast_helper.c | 44 ++- src/backend/catalog/system_views.sql | 20 ++ src/backend/postmaster/pgstat.c | 305 +- src/backend/utils/adt/pgstatfuncs.c | 72 + src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/catalog/pg_proc.dat | 25 ++ src/include/pgstat.h | 107 ++ src/test/regress/expected/rules.out | 17 + src/test/regress/expected/stats.out | 62 src/test/regress/sql/stats.sql| 28 ++ 14 files changed, 882 insertions(+), 9 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index afbb6c35e3..b2617cc941 100644 --- a/doc/src/sgml/config.sg
RE: Failed transaction statistics to measure the logical replication progress
On Monday, January 3, 2022 2:46 PM Hou, Zhijie/侯 志杰 wrote: > On Wednesday, December 22, 2021 6:14 PM Osumi, Takamichi > wrote: > > Attached the new patch v19. > Hi, > > Thanks for updating the patch. > > --- a/src/include/pgstat.h > +++ b/src/include/pgstat.h > @@ -15,6 +15,7 @@ > #include "portability/instr_time.h" > #include "postmaster/pgarch.h" /* for MAX_XFN_CHARS */ > #include "replication/logicalproto.h" > +#include "replication/worker_internal.h" > > I noticed that the patch includes "worker_internal.h " in pgstat.h. > I think it might be better to only include this file in pgstat.c. > And it seems we can access MyLogicalRepWorker directly in the following > functions instead of passing a parameter. > > +extern void pgstat_report_subworker_xact_end(LogicalRepWorker > *repWorker, > + >LogicalRepMsgType command, > + >bool bforce); > +extern void pgstat_send_subworker_xact_stats(LogicalRepWorker > *repWorker, > + >bool force); Hi, thank you for your review ! Both are fixed. Additionally, I modified related comments, the header comment of pgstat_send_subworker_xact_stats, by the change. One other new improvements in v20 is to have removed the 2nd argument of pgstat_send_subworker_xact_stats(). When we called it from commit-like operations, I passed 'false' without exceptions in v19 and noticed that could be removed. Also, there's a really minor alignment in the source codes. In pgstat.c, I placed pgstat_report_subworker_xact_end() after pgstat_report_subworker_error(), and pgstat_send_subworker_xact_stats() after pgstat_send_subscription_purge() and so on like the order of PgstatCollectorMain() and PgStat_Msg definition, because my patch's new functions are added after the existing functions for error handling functions of subscription workers. Lastly, I changed the error report in pgstat_report_subworker_xact_end() so that it can be more understandable easily and a bit modern. Kindly have a look at the attached version. Best Regards, Takamichi Osumi v20-0001-Fix-alignments-of-TAP-tests-for-pg_stat_subscrip.patch Description: v20-0001-Fix-alignments-of-TAP-tests-for-pg_stat_subscrip.patch v20-0002-Extend-pg_stat_subscription_workers-to-include-g.patch Description: v20-0002-Extend-pg_stat_subscription_workers-to-include-g.patch
Re: Documenting when to retry on serialization failure
On Thu, 16 Dec 2021 at 06:05, Greg Stark wrote: > So a lot of users are probably looking at something like "BEGIN; > SELECT create_customer_order(); COMMIT" and wondering why the > server can't handle automatically retrying the query if they get an > isolation failure. I agree with you that it would be desirable to retry for the simple case of an autocommit/single statement transaction run with default_transaction_isolation = 'serializability'. The most important question before we take further action is whether this would be correct to do so, in all cases. Some problem cases would help us decide either way. -- Simon Riggshttp://www.EnterpriseDB.com/
RE: Failed transaction statistics to measure the logical replication progress
On Friday, December 31, 2021 10:12 AM Tang, Haiying/唐 海英
wrote:
> On Wednesday, December 22, 2021 6:14 PM osumi.takami...@fujitsu.com
> wrote:
> >
> > Attached the new patch v19.
> >
>
> Thanks for your patch. I think it's better if you could add this patch to the
> commitfest.
> Here are some comments:
Thank you for your review !
I've created one entry in the next commitfest for this patch [1]
>
> 1)
> + commit_count bigint
> +
> +
> + Number of transactions successfully applied in this subscription.
> + Both COMMIT and COMMIT PREPARED increment this counter.
> +
> +
> ...
>
> I think the commands (like COMMIT, COMMIT PREPARED ...) can be
> surrounded with " ", thoughts?
Makes sense to me. Fixed.
Note that to the user perspective,
we should write only COMMIT and COMMIT PREPARED in the documentation.
Thus, I don't list up other commands.
I wrapped ROLLBACK PREPARED for abort_count as well.
> 2)
> +extern void pgstat_report_subworker_xact_end(LogicalRepWorker
> *repWorker,
> +
>LogicalRepMsgType command,
> +
>bool bforce);
>
> Should "bforce" be "force"?
Fixed the typo.
> 3)
> + * This should be called before the call of process_syning_tables() so
> + to not
>
> "process_syning_tables()" should be "process_syncing_tables()".
Fixed.
> 4)
> +void
> +pgstat_send_subworker_xact_stats(LogicalRepWorker *repWorker, bool
> +force) {
> + static TimestampTz last_report = 0;
> + PgStat_MsgSubWorkerXactEnd msg;
> +
> + if (!force)
> + {
> ...
> + if (!TimestampDifferenceExceeds(last_report, now,
> PGSTAT_STAT_INTERVAL))
> + return;
> + last_report = now;
> + }
> +
> ...
> + if (repWorker->commit_count == 0 && repWorker->abort_count ==
> 0)
> + return;
> ...
>
> I think it's better to check commit_count and abort_count first, then check if
> reach PGSTAT_STAT_INTERVAL.
> Otherwise if commit_count and abort_count are 0, it is possible that the value
> of last_report has been updated but it didn't send stats in fact. In this
> case,
> last_report is not the real time that send last message.
Yeah, agreed. This fix is right in terms of the variable name aspect.
The only scenario that we can take advantage of the previous implementation of
v19's pgstat_send_subworker_xact_stats() should be a case where
we execute a bunch of commit-like logical replication apply messages
within PGSTAT_STAT_INTERVAL intensively and continuously for long period,
because we check "repWorker->commit_count == 0 && repWorker->abort_count == 0"
just once before calling pgstat_send() in this case.
*But*, this scenario didn't look reasonable. In other words,
the way to call TimestampDifferenceExceeds() only if there's any need of
update for commit_count/abort_count looks less heavy.
Accordingly, I've fixed it as you suggested.
Also, I modified some comments in pgstat_send_subworker_xact_stats() for this
change.
Kindly have a look at the v20 shared in [2].
[1] - https://commitfest.postgresql.org/37/3504/
[2] -
https://www.postgresql.org/message-id/TYCPR01MB8373AB2AE1A6EC7B9E012519ED4A9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
Re: pg_dump/restore --no-tableam
On Mon, Jan 03, 2022 at 03:44:24PM -0600, Justin Pryzby wrote: > @cfbot: rebased Hmm. This could be useful to provide more control in some logical reload scenarios, so I'd agree to provide this switch. I'll look at the patch later.. -- Michael signature.asc Description: PGP signature
Re: Report checkpoint progress in server logs
On Wed, Dec 29, 2021 at 10:40:59AM -0500, Tom Lane wrote: > Magnus Hagander writes: >> I think the right choice to solve the *general* problem is the >> mentioned pg_stat_progress_checkpoints. > > +1 Agreed. I don't see why this would not work as there are PgBackendStatus entries for each auxiliary process. -- Michael signature.asc Description: PGP signature
Re: [PATCH] allow src/tools/msvc/*.bat files to be called from the root of the source tree
On Wed, Dec 29, 2021 at 09:48:14AM -0500, Andrew Dunstan wrote: > Seems reasonable. I don't see any reason not to do it for pgbison.bat > and pgflex.bat, just for the sake of consistency. Yeah, that would close the loop. Andrew, are you planning to check and apply this patch? -- Michael signature.asc Description: PGP signature
Re: Converting WAL to SQL
On Wed, Dec 29, 2021 at 08:50:23AM -0300, Fabrízio de Royes Mello wrote: > Try this: > https://github.com/michaelpq/pg_plugins/tree/main/decoder_raw You may want to be careful with this, and I don't know if anybody is using that for serious cases so some spots may have been missed. -- Michael signature.asc Description: PGP signature
Re: toast tables and toast indexes
On Tue, Dec 28, 2021 at 01:10:53PM +, Godfrin, Philippe E wrote: > While experimenting with toast tables I noticed that the toast index > lands in the same tablespace as the toast table itself. Is there a > way to make the toast indexes create in a different tablespace? No. See create_toast_table() where the toast table and its index use the same tablespace as the relation they depend on. Now, you could use allow_system_table_mods and an ALTER INDEX .. SET TABLESPACE to change that for the index, but this is a developer option and you should *not* use that for anything serious: https://www.postgresql.org/docs/devel/runtime-config-developer.html "Ill-advised use of this setting can cause irretrievable data loss or seriously corrupt the database system." -- Michael signature.asc Description: PGP signature
Re: [PATCH] allow src/tools/msvc/*.bat files to be called from the root of the source tree
On 1/4/22 07:20, Michael Paquier wrote: > On Wed, Dec 29, 2021 at 09:48:14AM -0500, Andrew Dunstan wrote: >> Seems reasonable. I don't see any reason not to do it for pgbison.bat >> and pgflex.bat, just for the sake of consistency. > Yeah, that would close the loop. Andrew, are you planning to check > and apply this patch? Sure, I can do that. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Converting WAL to SQL
On Tue, Jan 4, 2022 at 9:22 AM Michael Paquier wrote: > > On Wed, Dec 29, 2021 at 08:50:23AM -0300, Fabrízio de Royes Mello wrote: > > Try this: > > https://github.com/michaelpq/pg_plugins/tree/main/decoder_raw > > You may want to be careful with this, and I don't know if anybody is > using that for serious cases so some spots may have been missed. > I used it in the past during a major upgrade process from 9.2 to 9.6. What we did was decode the 9.6 wal files and apply transactions to the old 9.2 to keep it in sync with the new promoted version. This was our "rollback" strategy if something went wrong with the new 9.6 version. Regards, -- Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Re: daitch_mokotoff module
Andres Freund writes: > Hi, > > On 2022-01-02 21:41:53 -0500, Tom Lane wrote: >> ... so, that test case is guaranteed to fail in non-UTF8 encodings, >> I suppose? I wonder what the LANG environment is in that cfbot >> instance. > > LANG="en_US.UTF-8" > > But it looks to me like the problem is in the commit cfbot creates, rather > than the test run itself: > https://github.com/postgresql-cfbot/postgresql/commit/d5b4ec87cfd65dc08d26e1b789bd254405c90a66#diff-388d4bb360a3b24c425e29a85899315dc02f9c1dd9b9bc9aaa828876bdfea50aR56 > > Greetings, > > Andres Freund > > I have now separated out the UTF8-dependent tests, hopefully according to the current best practice (based on src/test/modules/test_regex/ and https://www.postgresql.org/docs/14/regress-variant.html). However I guess this won't make any difference wrt. actually running the tests, as long as there seems to be an encoding problem in the cfbot pipeline. Is there anything else I can do? Could perhaps fuzzystrmatch_utf8 simply be commented out from the Makefile for the time being? Best regards Dag Lem diff --git a/contrib/fuzzystrmatch/Makefile b/contrib/fuzzystrmatch/Makefile index 0704894f88..1d5bd84be8 100644 --- a/contrib/fuzzystrmatch/Makefile +++ b/contrib/fuzzystrmatch/Makefile @@ -3,14 +3,15 @@ MODULE_big = fuzzystrmatch OBJS = \ $(WIN32RES) \ + daitch_mokotoff.o \ dmetaphone.o \ fuzzystrmatch.o EXTENSION = fuzzystrmatch -DATA = fuzzystrmatch--1.1.sql fuzzystrmatch--1.0--1.1.sql +DATA = fuzzystrmatch--1.2.sql fuzzystrmatch--1.1--1.2.sql fuzzystrmatch--1.0--1.1.sql PGFILEDESC = "fuzzystrmatch - similarities and distance between strings" -REGRESS = fuzzystrmatch +REGRESS = fuzzystrmatch fuzzystrmatch_utf8 ifdef USE_PGXS PG_CONFIG = pg_config @@ -22,3 +23,8 @@ top_builddir = ../.. include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk endif + +daitch_mokotoff.o: daitch_mokotoff.h + +daitch_mokotoff.h: daitch_mokotoff_header.pl + perl $< > $@ diff --git a/contrib/fuzzystrmatch/daitch_mokotoff.c b/contrib/fuzzystrmatch/daitch_mokotoff.c new file mode 100644 index 00..1b7263c349 --- /dev/null +++ b/contrib/fuzzystrmatch/daitch_mokotoff.c @@ -0,0 +1,593 @@ +/* + * Daitch-Mokotoff Soundex + * + * Copyright (c) 2021 Finance Norway + * Author: Dag Lem + * + * This implementation of the Daitch-Mokotoff Soundex System aims at high + * performance. + * + * - The processing of each phoneme is initiated by an O(1) table lookup. + * - For phonemes containing more than one character, a coding tree is traversed + * to process the complete phoneme. + * - The (alternate) soundex codes are produced digit by digit in-place in + * another tree structure. + * + * References: + * + * https://www.avotaynu.com/soundex.htm + * https://www.jewishgen.org/InfoFiles/Soundex.html + * https://familypedia.fandom.com/wiki/Daitch-Mokotoff_Soundex + * https://stevemorse.org/census/soundex.html (dmlat.php, dmsoundex.php) + * https://github.com/apache/commons-codec/ (dmrules.txt, DaitchMokotoffSoundex.java) + * https://metacpan.org/pod/Text::Phonetic (DaitchMokotoff.pm) + * + * A few notes on other implementations: + * + * - All other known implementations have the same unofficial rules for "UE", + * these are also adapted by this implementation (0, 1, NC). + * - The only other known implementation which is capable of generating all + * correct soundex codes in all cases is the JOS Soundex Calculator at + * https://www.jewishgen.org/jos/jossound.htm + * - "J" is considered (only) a vowel in dmlat.php + * - The official rules for "RS" are commented out in dmlat.php + * - Identical code digits for adjacent letters are not collapsed correctly in + * dmsoundex.php when double digit codes are involved. E.g. "BESST" yields + * 744300 instead of 743000 as for "BEST". + * - "J" is considered (only) a consonant in DaitchMokotoffSoundex.java + * - "Y" is not considered a vowel in DaitchMokotoffSoundex.java + * + * Permission to use, copy, modify, and distribute this software and its + * documentation for any purpose, without fee, and without a written agreement + * is hereby granted, provided that the above copyright notice and this + * paragraph and the following two paragraphs appear in all copies. + * + * IN NO EVENT SHALL THE AUTHOR OR DISTRIBUTORS BE LIABLE TO ANY PARTY FOR + * DIRECT, INDIRECT, SPECIAL, INCIDENTAL, OR CONSEQUENTIAL DAMAGES, INCLUDING + * LOST PROFITS, ARISING OUT OF THE USE OF THIS SOFTWARE AND ITS + * DOCUMENTATION, EVEN IF THE AUTHOR OR DISTRIBUTORS HAVE BEEN ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * + * THE AUTHOR AND DISTRIBUTORS SPECIFICALLY DISCLAIM ANY WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY + * AND FITNESS FOR A PARTICULAR PURPOSE. THE SOFTWARE PROVIDED HEREUNDER IS + * ON AN "AS IS" BASIS, AND THE AUTHOR AND DISTRIBUTORS HAS NO OBLIGATIONS TO + * PROVIDE MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS. +*/
Re: SQL/JSON: functions
On 1/4/22 04:18, Himanshu Upadhyaya wrote:
> On Thu, Dec 16, 2021 at 3:06 AM Andrew Dunstan wrote:
>>
>>
>> SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
>> FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
>> (5,5)) kv(k, v);
>> ERROR: 22P02: invalid input syntax for type integer: "no"
>> LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
>> ^
>> LOCATION: pg_strtoint32, numutils.c:320
>>
>> Above call to JSON_OBJECTAGG is failing because we have the numeric
>> key, is not that it also needs to follow the same context of
>> converting key argument to text? or both(JSON_OBJECTAGG and
>> JSON_OBJECT) should not allow numeric keys in the JSON object and
>> allow date (if that is the only use case)?
>>
this error has nothing at all to do with the json code. You simply have
an invalid VALUES expression:
postgres=# select * FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2),
('foo', NULL), (5,5)) kv(k, v);
ERROR: invalid input syntax for type integer: "no"
LINE 1: select * FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2),...
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Index-only scan for btree_gist turns bpchar to char
Hello hackers,
While testing the index-only scan fix, I've discovered that replacing
the index-only scan with the index scan changes contrib/btree_gist
output because index-only scan for btree_gist returns a string without
padding.
A simple demonstration (based on btree_gist/sql/char.sql):
CREATE EXTENSION btree_gist;
CREATE TABLE chartmp (a char(32));
INSERT INTO chartmp VALUES('31b0');
CREATE INDEX charidx ON chartmp USING gist ( a );
SET enable_seqscan=off;
EXPLAIN VERBOSE SELECT *, octet_length(a) FROM chartmp WHERE a BETWEEN
'31a' AND '31c';
SELECT *, octet_length(a) FROM chartmp WHERE a BETWEEN '31a' AND '31c';
SET enable_indexonlyscan=off;
EXPLAIN VERBOSE SELECT *, octet_length(a) FROM chartmp WHERE a BETWEEN
'31a' AND '31c';
SELECT *, octet_length(a) FROM chartmp WHERE a BETWEEN '31a' AND '31c';
QUERY
PLAN
--
Index Only Scan using charidx on chartmp (cost=0.12..8.15 rows=1
width=136)
Index Cond: ((a >= '31a'::bpchar) AND (a <= '31c'::bpchar))
(2 rows)
a | octet_length
--+--
31b0 | 4
(1 row)
QUERY PLAN
-
Index Scan using charidx on chartmp (cost=0.12..8.15 rows=1 width=136)
Index Cond: ((a >= '31a'::bpchar) AND (a <= '31c'::bpchar))
(2 rows)
a | octet_length
--+--
31b0 | 32
(1 row)
It seems that loosing blank padding is incorrect (btree and btree_gin
preserve padding with index-only scan) but it's recorded in
contrib/btree_gist/expected/char.out.
Best regards,
Alexander
Re: [PATCH] Allow multiple recursive self-references
On 21.09.21 13:35, Denis Hirn wrote: Also, currently a query like this works [...] but this doesn't: WITH RECURSIVE t(n) AS ( SELECT n+1 FROM t WHERE n < 100 UNION ALL VALUES (1) ) SELECT sum(n) FROM t; With your patch, the second should also work, so let's show some tests for that as well. With just the tree rotation, the second query can not be fixed. The order of two nodes is never changed. And I think that this is a good thing. Consider the following query: WITH RECURSIVE t(n) AS ( VALUES (1) UNION ALL SELECT n+1 FROM t WHERE n < 100 UNION ALL VALUES (2) ) SELECT * FROM t LIMIT 100; If we'd just collect all non-recursive UNION branches, the semantics of the second query would change. But changing the semantics of a query (or preventing certain queries to be formulated at all) is not something I think this patch should do. Therfore – I think – it's appropriate that the second query fails. I have been studying this a bit more. I don't understand your argument here. Why would this query have different semantics than, say WITH RECURSIVE t(n) AS ( VALUES (1) UNION ALL VALUES (2) UNION ALL SELECT n+1 FROM t WHERE n < 100 ) SELECT * FROM t LIMIT 100; The order of UNION branches shouldn't be semantically relevant. I suppose you put the LIMIT clause in there to make some point, but I didn't get it. ;-) I also considered this example: WITH RECURSIVE t(n) AS ( (VALUES (1) UNION ALL VALUES (2)) UNION ALL SELECT n+1 FROM t WHERE n < 100 ) SELECT sum(n) FROM t; This works fine without and with your patch. This should be equivalent: WITH RECURSIVE t(n) AS ( VALUES (1) UNION ALL (VALUES (2) UNION ALL SELECT n+1 FROM t WHERE n < 100) ) SELECT sum(n) FROM t; But this runs forever in current PostgreSQL 14 and 15. I'd have expected your patch to convert this form to the previous form, but it doesn't. I'm having difficulties understanding which subset of cases your patch wants to address.
Re: [PATCH] Allow multiple recursive self-references
I have some separate questions on the executor changes. Basically, this seems the right direction, but I wonder if some things could be clarified. I wonder why in ExecWorkTableScan() and ExecReScanWorkTableScan() you call tuplestore_copy_read_pointer() instead of just tuplestore_select_read_pointer(). What is the special role of read pointer 0 that you are copying. Before your changes, it was just the implicit read pointer, but now that we have several, it would be good to explain their relationship. Also, the comment you deleted says "Therefore, we don't need a private read pointer for the tuplestore, nor do we need to tell tuplestore_gettupleslot to copy." You addressed the first part with the read pointer handling, but what about the second part? The tuplestore_gettupleslot() call in WorkTableScanNext() still has copy=false. Is this an oversight, or did you determine that copying is still not necessary?
Re: CREATEROLE and role ownership hierarchies
On Mon, Jan 3, 2022 at 5:08 PM Andrew Dunstan wrote:
>
>
> On 12/23/21 16:06, Joshua Brindle wrote:
> > On Tue, Dec 21, 2021 at 8:26 PM Mark Dilger
> > wrote:
> >>
> >>
> >>> On Dec 21, 2021, at 5:11 PM, Shinya Kato
> >>> wrote:
> >>>
> >>> I fixed the patches because they cannot be applied to HEAD.
> >> Thank you.
> > I reviewed and tested these and they LGTM. FYI the rebased v3 patches
> > upthread are raw diffs so git am won't apply them.
>
>
> That's not at all unusual. I normally apply patches just using
>
>patch -p 1 < $patchfile
>
> > I can add myself to
> > the CF as a reviewer if it is helpful.
>
>
> Please do.
I just ran across this and I don't know if it is intended behavior or
not, can you tell me why this happens?
postgres=> \du+
List of roles
Role name | Owner | Attributes
| Member of | Description
---+--++---+-
brindle | brindle | Password valid until 2022-01-05 00:00:00-05
| {}|
joshua| postgres | Create role
| {}|
postgres | postgres | Superuser, Create role, Create DB,
Replication, Bypass RLS | {}|
postgres=> \password
Enter new password for user "brindle":
Enter it again:
ERROR: role "brindle" with OID 16384 owns itself
Re: refactoring basebackup.c
On Mon, Jan 3, 2022 at 12:12 PM tushar wrote: > On 11/22/21 11:05 PM, Jeevan Ladhe wrote: > > Please find the lz4 compression patch here that basically has: > Please refer to this scenario , where --server-compression is only > compressing > base backup into lz4 format but not pg_wal directory > > [edb@centos7tushar bin]$ ./pg_basebackup -Ft --server-compression=lz4 > -Xstream -D foo > > [edb@centos7tushar bin]$ ls foo > backup_manifest base.tar.lz4 pg_wal.tar > > this same is valid for gzip as well if server-compression is set to gzip > > edb@centos7tushar bin]$ ./pg_basebackup -Ft --server-compression=gzip4 > -Xstream -D foo1 > > [edb@centos7tushar bin]$ ls foo1 > backup_manifest base.tar.gz pg_wal.tar > > if this scenario is valid then both the folders format should be in lz4 > format otherwise we should > get an error something like - not a valid option ? Before sending an email like this, it would be a good idea to read the documentation for the --server-compression option. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Proposal: remove obsolete hot-standby testing infrastructure
Alexander Lakhin writes: > 04.01.2022 00:50, Tom Lane wrote: >> The attached proposed patch removes some ancient infrastructure for >> manually testing hot standby. I doubt anyone has used this in years, >> because AFAICS there is nothing here that's not done better by the >> src/test/recovery TAP tests. (Or if there is, we ought to migrate >> it into the TAP tests.) > It's hardly that important, but we (Postgres Pro) run this test > regularly to check for primary-standby compatibility. It's useful when > checking binary packages from different minor versions. For example, we > setup postgresql-14.0 and postgresql-14.1 aside (renaming one > installation' directory and changing it's port) and perform the test. > What've found with it was e.g. incompatibility due to linkage of > different libicu versions (that was PgPro-only issue). I don't remember > whether we found something related to PostgreSQL itself, but we > definitely use this test and I'm not sure how to replace it in our setup > with a TAP test. On the other hand, testing binaries is not accustomed > in the community yet, so when such testing will be adopted, probably a > brand new set of tests should emerge. Oh, interesting. I definitely concur that testing compatibility of different builds or minor versions is an important use-case. And I concede that making src/test/recovery do it would be tricky and a bit out-of-scope. But having said that, the hs_standby_* scripts seem like a poor fit for the job too. AFAICS they don't really test any user data type except integer (so I'm surprised that they located an ICU incompatibility for you); and they spend a lot of effort on stuff that I doubt is relevant because it *is* covered by the TAP tests. If I were trying to test that topic using available spare parts, what I'd do is run the regular regression tests on the primary and see if the standby could track it. Maybe pg_dump from both servers afterwards and see if the results match, a la the pg_upgrade test. Bonus points for a script that could run some other pg_regress suite such as one of the contrib modules, because then you could check compatibility of those too. I'm happy to keep the hs_standby_* scripts if there's a live use-case for them; but I don't see what they're doing for you that wouldn't be done better by other pg_regress suites. regards, tom lane
Re: ICU for global collation
On 04.01.22 03:21, Julien Rouhaud wrote:
@@ -2774,6 +2776,7 @@ dumpDatabase(Archive *fout)
appendPQExpBuffer(dbQry, "SELECT tableoid, oid, datname, "
"(%s datdba) AS dba, "
"pg_encoding_to_char(encoding) AS
encoding, "
+ "datcollprovider, "
This needs to be in a new pg 15+ branch, not in the pg 9.3+.
ok
- if (!lc_collate_is_c(collid) && collid != DEFAULT_COLLATION_OID)
- mylocale = pg_newlocale_from_collation(collid);
+ if (!lc_collate_is_c(collid))
+ {
+ if (collid != DEFAULT_COLLATION_OID)
+ mylocale = pg_newlocale_from_collation(collid);
+ else if (default_locale.provider == COLLPROVIDER_ICU)
+ mylocale = &default_locale;
+ }
There are really a lot of places with this new code. Maybe it could be some
new function/macro to wrap that for the normal case (e.g. not formatting.c)?
Right, we could just put this into pg_newlocale_from_collation(), but
the comment there says
* In fact, they shouldn't call this function at all when they are dealing
* with the default locale. That can save quite a bit in hotspots.
I don't know how to assess that.
We could pack this into a macro or inline function if we are concerned
about this.
Re: SKIP LOCKED assert triggered
On 2022-Jan-03, Alvaro Herrera wrote: > What I don't understand is why hasn't this been reported already: this > bug is pretty old. My only explanation is that nobody runs sufficiently- > concurrent load with SKIP LOCKED in assert-enabled builds. Pushed, thanks Simon for reporting this problem. > [1] > https://www.postgresql.org/message-id/flat/CADLWmXUvd5Z%2BcFczi6Zj1WcTrXzipgP-wj0pZOWSaRUy%3DF0omQ%40mail.gmail.com Heh, I deleted a paragraph from my previous email and forgot to remove the footnote that it referenced. -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "I love the Postgres community. It's all about doing things _properly_. :-)" (David Garamond)
Re: SKIP LOCKED assert triggered
On 2022-Jan-04, Alvaro Herrera wrote: > On 2022-Jan-03, Alvaro Herrera wrote: > > > What I don't understand is why hasn't this been reported already: this > > bug is pretty old. My only explanation is that nobody runs sufficiently- > > concurrent load with SKIP LOCKED in assert-enabled builds. > > Pushed, thanks Simon for reporting this problem. Wow, what an embarrassing problem this fix has. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "Pensar que el espectro que vemos es ilusorio no lo despoja de espanto, sólo le suma el nuevo terror de la locura" (Perelandra, C.S. Lewis)
Re: SKIP LOCKED assert triggered
Alvaro Herrera writes: > Pushed, thanks Simon for reporting this problem. Umm ... Assert(TM_WouldBlock || !(tuple->t_data->t_infomask & HEAP_XMAX_INVALID)); AFAICS, this assertion condition is constant-true, because TM_WouldBlock is a nonzero constant. Perhaps you meant Assert(result == TM_WouldBlock || !(tuple->t_data->t_infomask & HEAP_XMAX_INVALID)); ? I'd be inclined to format it more like the adjacent Assert, too. regards, tom lane
Re: pg_walinspect - a new extension to get raw WAL data and WAL stats
On Thu, Nov 25, 2021 at 5:54 PM Bharath Rupireddy wrote: > > On Thu, Nov 25, 2021 at 3:49 PM Bharath Rupireddy > wrote: > > > > > > Thanks all. Here's the v1 patch set for the new extension pg_walinspect. > > > Note that I didn't include the documentation part now, I will be doing it > > > a bit later. > > > > > > Please feel free to review and provide your thoughts. > > > > The v1 patch set was failing to compile on Windows. Here's the v2 > > patch set fixing that. > > I forgot to specify this: the v1 patch set was failing to compile on > Windows with errors shown at [1]. Thanks to Julien Rouhaud who > suggested to use PGDLLIMPORT in an off-list discussion. > > [1] (Link target) -> > pg_walinspect.obj : error LNK2001: unresolved external symbol > forkNames [C:\Users\bhara\postgres\pg_walinspect.vcxproj] > pg_walinspect.obj : error LNK2001: unresolved external symbol > pg_comp_crc32c [C:\Users\bhara\postgres\pg_walinspect.vcxproj] > pg_walinspect.obj : error LNK2001: unresolved external symbol > wal_segment_size [C:\Users\bhara\postgres\pg_walinspect.vcxproj] > pg_walinspect.obj : error LNK2001: unresolved external symbol > RmgrTable [C:\Users\bhara\postgres\pg_walinspect.vcxproj] > .\Release\pg_walinspect\pg_walinspect.dll : fatal error LNK1120: 4 > unresolved externals [C:\Users\bhara\postgres\pg_walinspect.vcxproj] > > 5 Error(s) Here's the v3 patch-set with fixes for the compiler warnings reported in the cf bot at https://cirrus-ci.com/task/4979131497578496?logs=gcc_warning#L506. Please review. Regards, Bharath Rupireddy. v3-0001-pg_walinspect.patch Description: Binary data v3-0001-pg_walinspect-tests.patch Description: Binary data
Re: SKIP LOCKED assert triggered
On Tue, Jan 04, 2022 at 11:15:30AM -0500, Tom Lane wrote: > Alvaro Herrera writes: > > Pushed, thanks Simon for reporting this problem. > > Umm ... > >Assert(TM_WouldBlock || !(tuple->t_data->t_infomask & > HEAP_XMAX_INVALID)); > > AFAICS, this assertion condition is constant-true, The new cfbot was failing like this https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/36/3423 https://cirrus-ci.com/build/5839382107127808 [22:52:27.978] heapam.c:4754:24: error: converting the enum constant to a boolean [-Werror,-Wint-in-bool-context]
Re: CREATEROLE and role ownership hierarchies
> On Jan 4, 2022, at 6:35 AM, Joshua Brindle > wrote: > > I just ran across this and I don't know if it is intended behavior or > not > postgres=> \password > Enter new password for user "brindle": > Enter it again: > ERROR: role "brindle" with OID 16384 owns itself No, that looks like a bug. Thanks for reviewing! — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
biblio.sgml dead link
On master, doc/src/sgml/biblio.sgml has a biblioentry for a pdf from ISO: "Information technology — Database languages — SQL Technical Reports — Part SQL Notation support 6: (JSON) for JavaScript Object" That pdf was a 2017 edition but the url now points to .zip that no longer exists. The replacement is a ~200 euro pdf (2021). I'd be thankful if someone would send the pdf to me; maybe I can update my JSON tests. And we should remove that entry from the bibliography (or have it point to the new page [1]). Erik Rijkers [1] https://www.iso.org/standard/78937.html
Re: Proposal: remove obsolete hot-standby testing infrastructure
04.01.2022 18:33, Tom Lane wrote: > Alexander Lakhin writes: >> It's hardly that important, but we (Postgres Pro) run this test >> regularly to check for primary-standby compatibility. It's useful when >> checking binary packages from different minor versions. For example, we >> setup postgresql-14.0 and postgresql-14.1 aside (renaming one >> installation' directory and changing it's port) and perform the test. >> What've found with it was e.g. incompatibility due to linkage of >> different libicu versions (that was PgPro-only issue). I don't remember >> whether we found something related to PostgreSQL itself, but we >> definitely use this test and I'm not sure how to replace it in our setup >> with a TAP test. On the other hand, testing binaries is not accustomed >> in the community yet, so when such testing will be adopted, probably a >> brand new set of tests should emerge. > Oh, interesting. I definitely concur that testing compatibility of > different builds or minor versions is an important use-case. And > I concede that making src/test/recovery do it would be tricky and > a bit out-of-scope. But having said that, the hs_standby_* scripts > seem like a poor fit for the job too. AFAICS they don't really > test any user data type except integer (so I'm surprised that they > located an ICU incompatibility for you); and they spend a lot of > effort on stuff that I doubt is relevant because it *is* covered > by the TAP tests. An ICU incompatibility was detected due to our invention [1] "default collation" that is checked upon connection (before any query processing): --- C:/tmp/.../src/test/regress/expected/hs_standby_check.out 2021-10-14 04:07:38.0 +0200 +++ C:/tmp/.../src/test/regress/results/hs_standby_check.out 2021-10-14 06:06:12.004043500 +0200 @@ -1,3 +1,6 @@ +WARNING: collation "default" has version mismatch +DETAIL: The collation in the database was created using version 153.64, but the operating system provides version 153.14. +HINT: Check all objects affected by this collation and run ALTER COLLATION pg_catalog."default" REFRESH VERSION -- -- Hot Standby tests -- I admit that we decided to use this test mainly because it exists and described in the documentation, not because it seemed very useful. It's usage increased test coverage without a doubt, as it requires a rather non-trivial setup (similar setups performed by TAP tests, but not with pre-packaged binaries). > If I were trying to test that topic using available spare parts, > what I'd do is run the regular regression tests on the primary > and see if the standby could track it. Maybe pg_dump from both > servers afterwards and see if the results match, a la the pg_upgrade > test. Bonus points for a script that could run some other pg_regress > suite such as one of the contrib modules, because then you could > check compatibility of those too. Thanks for the idea! We certainly will implement something like that when we start testing packages for v15. We've already learned to compare dumps before/after minor upgrade, so we could reuse that logic for this test too. > I'm happy to keep the hs_standby_* scripts if there's a live use-case > for them; but I don't see what they're doing for you that wouldn't be > done better by other pg_regress suites. Yes, I will not miss the test in case you will remove it. I just wanted to mention that we use(d) it in our testing more or less successfully. [1] https://www.postgresql.org/message-id/37A534BE-CBF7-467C-B096-0AAD25091A9F%40yandex-team.ru Best regards, Alexander
Re: Index-only scan for btree_gist turns bpchar to char
Alexander Lakhin writes: > While testing the index-only scan fix, I've discovered that replacing > the index-only scan with the index scan changes contrib/btree_gist > output because index-only scan for btree_gist returns a string without > padding. Ugh, yeah. This seems to be because gbt_bpchar_compress() strips trailing spaces (using rtrim1) before storing the value. The idea evidently is to simplify gbt_bpchar_consistent, but it's not acceptable if the opclass is supposed to support index-only scan. I see two ways to fix this: * Disallow index-only scan, by removing the fetch function for this opclass. This'd require a module version bump, so people wouldn't get that fix automatically. * Change gbt_bpchar_compress to not trim spaces (it becomes just like gbt_text_compress), and adapt gbt_bpchar_consistent to cope. This does nothing for the problem immediately, unless you REINDEX affected indexes --- but over time an index's entries would get replaced with untrimmed versions. I also wondered if we could make the fetch function reconstruct the padding, but it doesn't seem to have access to the necessary info. regards, tom lane
Re: Add 64-bit XIDs into PostgreSQL 15
Greetings, * Maxim Orlov (orlo...@gmail.com) wrote: > Long time wraparound was a really big pain for highly loaded systems. One > source of performance degradation is the need to vacuum before every > wraparound. > And there were several proposals to make XIDs 64-bit like [1], [2], [3] and > [4] to name a few. > > The approach [2] seems to have stalled on CF since 2018. But meanwhile it > was successfully being used in our Postgres Pro fork all time since then. > We have hundreds of customers using 64-bit XIDs. Dozens of instances are > under load that require wraparound each 1-5 days with 32-bit XIDs. > It really helps the customers with a huge transaction load that in the case > of 32-bit XIDs could experience wraparounds every day. So I'd like to > propose this approach modification to CF. > > PFA updated working patch v6 for PG15 development cycle. > It is based on a patch by Alexander Korotkov version 5 [5] with a few > fixes, refactoring and was rebased to PG15. Just to confirm as I only did a quick look- if a transaction in such a high rate system lasts for more than a day (which certainly isn't completely out of the question, I've had week-long transactions before..), and something tries to delete a tuple which has tuples on it that can't be frozen yet due to the long-running transaction- it's just going to fail? Not saying that I've got any idea how to fix that case offhand, and we don't really support such a thing today as the server would just stop instead, but if I saw something in the release notes talking about PG moving to 64bit transaction IDs, I'd probably be pretty surprised to discover that there's still a 32bit limit that you have to watch out for or your system will just start failing transactions. Perhaps that's a worthwhile tradeoff for being able to generally avoid having to vacuum and deal with transaction wrap-around, but I have to wonder if there might be a better answer. Of course, also wonder about how we're going to document and monitor for this potential issue and what kind of corrective action will be needed (kill transactions older than a cerain amount of transactions..?). Thanks, Stephen signature.asc Description: PGP signature
Re: SKIP LOCKED assert triggered
On Tue, 4 Jan 2022 at 16:15, Tom Lane wrote: > > Alvaro Herrera writes: > > Pushed, thanks Simon for reporting this problem. And causing another; my bad, apologies. > Umm ... > >Assert(TM_WouldBlock || !(tuple->t_data->t_infomask & > HEAP_XMAX_INVALID)); > > AFAICS, this assertion condition is constant-true, > because TM_WouldBlock is a nonzero constant. Perhaps you meant > >Assert(result == TM_WouldBlock || !(tuple->t_data->t_infomask & > HEAP_XMAX_INVALID)); Yes, I think that's what I meant. -- Simon Riggshttp://www.EnterpriseDB.com/
Re: daitch_mokotoff module
On Wed, Jan 5, 2022 at 2:49 AM Dag Lem wrote: > However I guess this won't make any difference wrt. actually running the > tests, as long as there seems to be an encoding problem in the cfbot Fixed -- I told it to pull down patches as binary, not text. Now it makes commits that look healthier, and so far all the Unix systems have survived CI: https://github.com/postgresql-cfbot/postgresql/commit/79700efc61d15c2414b8450a786951fa9308c07f http://cfbot.cputube.org/dag-lem.html
Re: CREATEROLE and role ownership hierarchies
> On Jan 4, 2022, at 9:07 AM, Mark Dilger wrote: > > No, that looks like a bug. I was able to reproduce that using REASSIGN OWNED BY to cause a user to own itself. Is that how you did it, or is there yet another way to get into that state? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: psql: \dl+ to list large objects privileges
Justin Pryzby writes:
> I suggest to move the function in a separate 0001 commit, which makes no code
> changes other than moving from one file to another.
> A committer would probably push them as a single patch, but this makes it
> easier to read and review the changes in 0002.
Yeah, I agree with that idea. It's really tough to notice small changes
by hand when the entire code block has been moved somewhere else.
> Since a few weeks ago, psql no longer supports server versions before 9.2, so
> the "if" branch can go away.
And, in fact, the patch is no longer applying per the cfbot, because
that hasn't been done.
To move things along a bit, I split the patch more or less as Justin
suggests and brought it up to HEAD. I did not study the command.c
changes, but the rest of it seems okay, with one exception: I don't like
the test case much at all. For one thing, it will fail in the buildfarm
because you didn't adhere to the convention that roles created by a
regression test must be named regress_something. For another, there's
considerable overlap with testing done in largeobject.sql, which
already creates some commented large objects. That means there's
an undesirable ordering dependency --- if somebody wanted to run
largeobject.sql first, the expected output of psql.sql would change.
I wonder if we shouldn't put these new tests in largeobject.sql instead.
(That would mean there are two expected-files to change, which is a bit
tedious, but it's not very hard.)
regards, tom lane
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index fb3bab9494..0d64757899 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -811,7 +811,7 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = describeRoles(pattern, show_verbose, show_system);
break;
case 'l':
-success = do_lo_list();
+success = listLargeObjects();
break;
case 'L':
success = listLanguages(pattern, show_verbose, show_system);
@@ -1963,7 +1963,7 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
}
else if (strcmp(cmd + 3, "list") == 0)
- success = do_lo_list();
+ success = listLargeObjects();
else if (strcmp(cmd + 3, "unlink") == 0)
{
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index c28788e84f..0d92f3a80b 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6455,3 +6455,39 @@ listOpFamilyFunctions(const char *access_method_pattern,
PQclear(res);
return true;
}
+
+/*
+ * \dl or \lo_list
+ * Lists large objects
+ */
+bool
+listLargeObjects(void)
+{
+ PGresult *res;
+ char buf[1024];
+ printQueryOpt myopt = pset.popt;
+
+ snprintf(buf, sizeof(buf),
+ "SELECT oid as \"%s\",\n"
+ " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
+ " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
+ " FROM pg_catalog.pg_largeobject_metadata "
+ " ORDER BY oid",
+ gettext_noop("ID"),
+ gettext_noop("Owner"),
+ gettext_noop("Description"));
+
+ res = PSQLexec(buf);
+ if (!res)
+ return false;
+
+ myopt.topt.tuples_only = false;
+ myopt.nullPrint = NULL;
+ myopt.title = _("Large objects");
+ myopt.translate_header = true;
+
+ printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+
+ PQclear(res);
+ return true;
+}
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 71b320f1fc..3a55e1e4ed 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -139,5 +139,7 @@ extern bool listOpFamilyOperators(const char *accessMethod_pattern,
extern bool listOpFamilyFunctions(const char *access_method_pattern,
const char *family_pattern, bool verbose);
+/* \dl or \lo_list */
+extern bool listLargeObjects(void);
#endif /* DESCRIBE_H */
diff --git a/src/bin/psql/large_obj.c b/src/bin/psql/large_obj.c
index 10e47c87ac..243875be83 100644
--- a/src/bin/psql/large_obj.c
+++ b/src/bin/psql/large_obj.c
@@ -262,42 +262,3 @@ do_lo_unlink(const char *loid_arg)
return true;
}
-
-
-
-/*
- * do_lo_list()
- *
- * Show all large objects in database with comments
- */
-bool
-do_lo_list(void)
-{
- PGresult *res;
- char buf[1024];
- printQueryOpt myopt = pset.popt;
-
- snprintf(buf, sizeof(buf),
- "SELECT oid as \"%s\",\n"
- " pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
- " pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
- " FROM pg_catalog.pg_largeobject_metadata "
- " ORDER BY oid",
- gettext_noop("ID"),
- gettext_noop("Owner"),
- gettext_noop("Description"));
-
- res = PSQLexec(buf);
- if (!res)
- return false;
-
- myopt.topt.tuples_only = false;
- myopt.nullPrint = NULL;
- myopt.title = _("Large objects");
- myopt.translate_header = true;
-
- printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
-
- PQclear(res);
- return true;
-}
diff --git a/src/bin/psql/large_obj.h b/src/bin/psql/large_obj.h
index 003acbf52c..3172a
Re: CREATEROLE and role ownership hierarchies
On Tue, Jan 4, 2022 at 3:39 PM Mark Dilger wrote: > > > > > On Jan 4, 2022, at 9:07 AM, Mark Dilger > > wrote: > > > > No, that looks like a bug. > > I was able to reproduce that using REASSIGN OWNED BY to cause a user to own > itself. Is that how you did it, or is there yet another way to get into that > state? I did: ALTER ROLE brindle OWNER TO brindle;
Re: [PATCH v2] use has_privs_for_role for predefined roles
"Bossart, Nathan" writes: > On 11/12/21, 12:34 PM, "Joshua Brindle" > wrote: >> All of these and also adminpack.sgml updated. I think that is all of >> them but docs broken across lines and irregular wording makes it >> difficult. > LGTM. I've marked this as ready-for-committer. This needs another rebase --- it's trying to adjust file_fdw/output/file_fdw.source, which no longer exists (fix the regular file_fdw.out, instead). regards, tom lane
Re: preserve timestamps when installing headers
Peter Eisentraut writes: > On 06.12.21 12:15, Michael Paquier wrote: >> FWIW, I am not on board with changing build semantics or any >> assumptions the header installation relies on either, but I could see >> a point in switching back to INSTALL_DATA instead of cp to be >> consistent with the rest of the build, iff the argument made back in >> 2005 about the performance of this code path does not hold anymore. > I think you will find that it is still very slow. That would likely depend on whether configure had found a suitable "install" program or decided to fall back on config/install-sh. The latter will definitely be horribly slow, but C-coded install utilities are probably no slower than "cp". However, there's another problem with using INSTALL_DATA as a solution to this issue: why would you expect that to preserve timestamps? install-sh won't. I see that /usr/bin/install (which configure picks on my RHEL box) won't preserve them by default, but it has a -p option to do so. I would not bet on that being portable to all of the myriad of foo-install programs that configure will accept, though. On the whole, I think we should just reject this proposal and move on. The portability hazards seem significant, and it's really unclear to me what the advantages are (per Peter's earlier comment). regards, tom lane
Re: [PATCH v2] use has_privs_for_role for predefined roles
On Tue, Jan 4, 2022 at 3:56 PM Tom Lane wrote: > > "Bossart, Nathan" writes: > > On 11/12/21, 12:34 PM, "Joshua Brindle" > > wrote: > >> All of these and also adminpack.sgml updated. I think that is all of > >> them but docs broken across lines and irregular wording makes it > >> difficult. > > > LGTM. I've marked this as ready-for-committer. > > This needs another rebase --- it's trying to adjust > file_fdw/output/file_fdw.source, which no longer exists > (fix the regular file_fdw.out, instead). > > regards, tom lane Attached, thanks v4-0001-use-has_privs_for_roles-for-predefined-role-checks.patch Description: Binary data
Re: A spot of redundant initialization of brin memtuple
Richard Guo writes: > On Mon, Nov 22, 2021 at 12:52 PM Bharath Rupireddy < > bharath.rupireddyforpostg...@gmail.com> wrote: >> Thanks. The patch looks good to me. Let's add it to the commitfest to >> not lose track of it. > Done. Here it is: > https://commitfest.postgresql.org/36/3424/ Pushed, thanks for the patch. regards, tom lane
Re: Add 64-bit XIDs into PostgreSQL 15
On 1/4/22, 2:35 PM, "Stephen Frost" wrote: >> >>Not saying that I've got any idea how to fix that case offhand, and we >>don't really support such a thing today as the server would just stop >>instead, ... >> Perhaps that's a >> worthwhile tradeoff for being able to generally avoid having to vacuum >> and deal with transaction wrap-around, but I have to wonder if there >> might be a better answer. >> For the target use cases that PostgreSQL is designed for, it's a very worthwhile tradeoff in my opinion. Such long-running transactions need to be killed. Re: -- If after upgrade page has no free space for special data, tuples are converted to "double xmax" format: xmin became virtual FrozenTransactionId, xmax occupies the whole 64bit. Page converted to new format when vacuum frees enough space. I'm concerned about the maintainability impact of having 2 new on-disk page formats. It's already complex enough with XIDs and multixact-XIDs. If the lack of space for the two epochs in the special data area is a problem only in an upgrade scenario, why not resolve the problem before completing the upgrade process like a kind of post-process pg_repack operation that converts all "double xmax" pages to the "double-epoch" page format? i.e. maybe the "double xmax" representation is needed as an intermediate representation during upgrade, but after upgrade completes successfully there are no pages with the "double-xmax" representation. This would eliminate a whole class of coding errors and would make the code dealing with 64-bit XIDs simpler and more maintainable. /Jim
Re: Consider parallel for lateral subqueries with limit
Greg Nancarrow writes: > The patch LGTM. > I have set the status to "Ready for Committer". I don't really see why this patch is even a little bit safe. The argument for it seems to be that a lateral subquery will necessarily be executed in such a way that each complete iteration of the subquery, plus joining to its outer rel, happens within a single worker ... but where is the guarantee of that? Once you've marked the rel as parallel-safe, the planner is free to consider all sorts of parallel join structures. I'm afraid this would be easily broken as soon as you look at cases with three or more rels. Or maybe even just two. The reason for the existing restriction boils down to this structure being unsafe: Gather NestLoop Scan ... Limit Scan ... and I don't see how the existence of a lateral reference makes it any safer. regards, tom lane
Re: [PATCH] Accept IP addresses in server certificate SANs
On Thu, 2021-12-16 at 18:44 +, Jacob Champion wrote:
> It sounds like both you and Andrew might be comfortable with that same
> behavior? I think it looks like a sane solution, so I'll implement that
> and we can see what it looks like. (My work on this will be paused over
> the end-of-year holidays.)
v2 implements the discussed CN/SAN fallback behavior and should fix the
build on Windows. Still TODO is the internal pg_inet_pton() refactoring
that you asked for; I'm still deciding how best to approach it.
Changes only in since-v1.diff.txt.
Thanks,
--Jacob
diff --git a/src/interfaces/libpq/fe-secure-common.c
b/src/interfaces/libpq/fe-secure-common.c
index 38ee65abf2..796b78e348 100644
--- a/src/interfaces/libpq/fe-secure-common.c
+++ b/src/interfaces/libpq/fe-secure-common.c
@@ -28,14 +28,6 @@
#include "port.h"
#include "pqexpbuffer.h"
-/*
- * In a frontend build, we can't include inet.h, but we still need to have
- * sensible definitions of these two constants. Note that pg_inet_net_ntop()
- * assumes that PGSQL_AF_INET is equal to AF_INET.
- */
-#define PGSQL_AF_INET (AF_INET + 0)
-#define PGSQL_AF_INET6 (AF_INET + 1)
-
/*
* Check if a wildcard certificate matches the server hostname.
*
diff --git a/src/interfaces/libpq/fe-secure-common.h
b/src/interfaces/libpq/fe-secure-common.h
index a090a92f60..29d4f54230 100644
--- a/src/interfaces/libpq/fe-secure-common.h
+++ b/src/interfaces/libpq/fe-secure-common.h
@@ -18,6 +18,14 @@
#include "libpq-fe.h"
+/*
+ * In a frontend build, we can't include inet.h, but we still need to have
+ * sensible definitions of these two constants. Note that pg_inet_net_ntop()
+ * assumes that PGSQL_AF_INET is equal to AF_INET.
+ */
+#define PGSQL_AF_INET (AF_INET + 0)
+#define PGSQL_AF_INET6 (AF_INET + 1)
+
extern int pq_verify_peer_name_matches_certificate_name(PGconn *conn,
const char *namedata, size_t namelen,
char **store_name);
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
index 16c5ff9223..479f63197a 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -548,6 +548,16 @@ openssl_verify_peer_name_matches_certificate_ip(PGconn
*conn,
return pq_verify_peer_name_matches_certificate_ip(conn, (const char *)
addrdata, len, store_name);
}
+static bool
+is_ip_address(const char *host)
+{
+ struct in_addr dummy4;
+ unsigned char dummy6[16];
+
+ return inet_aton(host, &dummy4)
+ || (pg_inet_net_pton(PGSQL_AF_INET6, host, dummy6, -1) == 128);
+}
+
/*
* Verify that the server certificate matches the hostname we connected to.
*
@@ -561,7 +571,36 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn
*conn,
STACK_OF(GENERAL_NAME) * peer_san;
int i;
int rc = 0;
- boolhas_dnsname = false;
+ char *host = conn->connhost[conn->whichhost].host;
+ int host_type;
+ boolcheck_cn = true;
+
+ Assert(host && host[0]); /* should be guaranteed by caller */
+
+ /*
+* We try to match the NSS behavior here, which is a slight departure
from
+* the spec but seems to make more intuitive sense:
+*
+* If connhost contains a DNS name, and the certificate's SANs contain
any
+* dNSName entries, then we'll ignore the Subject Common Name entirely;
+* otherwise, we fall back to checking the CN. (This behavior matches
the
+* RFC.)
+*
+* If connhost contains an IP address, and the SANs contain iPAddress
+* entries, we again ignore the CN. Otherwise, we allow the CN to match,
+* EVEN IF there is a dNSName in the SANs. (RFC 6125 prohibits this: "A
+* client MUST NOT seek a match for a reference identifier of CN-ID if
the
+* presented identifiers include a DNS-ID, SRV-ID, URI-ID, or any
+* application-specific identifier types supported by the client.")
+*
+* NOTE: Prior versions of libpq did not consider iPAddress entries at
all,
+* so this new behavior might break a certificate that has different IP
+* addresses in the Subject CN and the SANs.
+*/
+ if (is_ip_address(host))
+ host_type = GEN_IPADD;
+ else
+ host_type = GEN_DNS;
/*
* First, get the Subject Alternative Names (SANs) from the certificate,
@@ -579,9 +618,11 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn
*conn,
const GENERAL_NAME *name =
sk_GENERAL_NAME_value(peer_san, i);
char *alt_name = NULL;
+
Re: Patch to avoid orphaned dependencies
Hi, For genam.c: + UseDirtyCatalogSnapshot = dirtysnap; + Does the old value of UseDirtyCatalogSnapshot need to be restored at the end of the func ? +systable_recheck_tuple(SysScanDesc sysscan, HeapTuple tup, bool dirtysnap) Considering that parameter dirtysnap is a bool, I think it should be named isdirtysnap so that its meaning can be distinguished from: + Snapshot dirtySnapshot; + UseDirtyCatalogSnapshot = true; + + dirtySnapshot = GetCatalogSnapshot(RelationGetRelid(*depRel)); I tend to think that passing usedirtysnap (bool parameter) to GetCatalogSnapshot() would be more flexible than setting global variable. Cheers
Re: pg_stat_statements and "IN" conditions
Dmitry Dolgov <9erthali...@gmail.com> writes: > And now for something completely different, here is a new patch version. > It contains a small fix for one problem we've found during testing (one > path code was incorrectly assuming find_const_walker results). I've been saying from day one that pushing the query-hashing code into the core was a bad idea, and I think this patch perfectly illustrates why. We can debate whether the rules proposed here are good for pg_stat_statements or not, but it seems inevitable that they will be a disaster for some other consumers of the query hash. In particular, dropping external parameters from the hash seems certain to break something for somebody --- do you really think that a query with two int parameters is equivalent to one with five float parameters for all query-identifying purposes? I can see the merits of allowing different numbers of IN elements to be considered equivalent for pg_stat_statements, but this patch seems to go far beyond that basic idea, and I fear the side-effects will be very bad. Also, calling eval_const_expressions in the query jumbler is flat out unacceptable. There is way too much code that could be reached that way (more or less the entire executor, to start with). I don't have a lot of faith that it'd never modify the input tree, either. regards, tom lane
Re: using extended statistics to improve join estimates
On 2022-01-01 18:21:06 +0100, Tomas Vondra wrote:
> Here's an updated patch, rebased and fixing a couple typos reported by
> Justin Pryzby directly.
FWIW, cfbot reports a few compiler warnings:
https://cirrus-ci.com/task/6067262669979648?logs=gcc_warning#L505
[18:52:15.132] time make -s -j${BUILD_JOBS} world-bin
[18:52:22.697] mcv.c: In function ‘mcv_combine_simple’:
[18:52:22.697] mcv.c:2787:7: error: ‘reverse’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]
[18:52:22.697] 2787 |if (reverse)
[18:52:22.697] | ^
[18:52:22.697] mcv.c:2766:27: error: ‘index’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]
[18:52:22.697] 2766 | if (mcv->items[i].isnull[index])
[18:52:22.697] | ^
Greetings,
Andres Freund
Re: [PoC] Delegating pg_ident to a third party
On Mon, 2022-01-03 at 19:42 -0500, Stephen Frost wrote: > * Jacob Champion (pchamp...@vmware.com) wrote: > > > > That last point was my motivation for the authn_id patch [1] -- so that > > auditing could see the actual user _and_ the generic role. The > > information is already there to be used, it's just not exposed to the > > stats framework yet. > > While that helps, and I generally support adding that information to the > logs, it's certainly not nearly as good or useful as having the actual > user known to the database. Could you talk more about the use cases for which having the "actual user" is better? From an auditing perspective I don't see why "authenticated as ja...@example.net, logged in as admin" is any worse than "logged in as jacob". > > Forcing one role per individual end user is wasteful and isn't really > > making good use of the role-based system that you already have. > > Generally speaking, when administering hundreds or thousands of users, > > people start dividing them up into groups as opposed to dealing with > > them individually. So I don't think new features should be taking away > > flexibility in this area -- if one role per user already works well for > > you, great, but don't make everyone do the same. > > Using the role system we have to assign privileges certainly is useful > and sensible, of course, though I don't see where you've actually made > an argument for why one role per individual is somehow wasteful or > somehow takes away from the role system that we have for granting > rights. I was responding more to your statement that "Being able to have roles and memberships automatically created is much more the direction that I'd say we should be going in". It's not that one-role-per-user is inherently wasteful, but forcing role proliferation where it's not needed is. If all users have the same set of permissions, there doesn't need to be more than one role. But see below. > I'm also not suggesting that we make everyone do the same > thing, indeed, later on I was supportive of having an external system > provide the mapping. Here, I'm just making the point that we should > also be looking at automatic role/membership creation. Gotcha. Agreed; that would open up the ability to administer role privileges externally too, which would be cool. That could be used in tandem with something like this patchset. > > > I'd go a step further and suggest that the way to do this is with a > > > background worker that's started up and connects to an LDAP > > > infrastructure and listens for changes, allowing the system to pick up > > > on new roles/memberships as soon as they're created in the LDAP > > > environment. That would then be controlled by appropriate settings in > > > postgresql.conf/.auto.conf. > > > > This is roughly what you can already do with existing (third-party) > > tools, and that approach isn't scaling out in practice for some of our > > existing customers. The load on the central server, for thousands of > > idle databases dialing in just to see if there are any new users, is > > huge. > > If you're referring specifically to cron-based tools which are > constantly hammering on the LDAP servers running the same queries over > and over, sure, I agree that that's creating load on the LDAP > infrastructure (though, well, it was kind of designed to be very > scalable for exactly that kind of load, no? So I'm not really sure why > that's such an issue..). I don't have hands-on experience here -- just going on what I've been told via field/product teams -- but it seems to me that there's a big difference between asking an LDAP server to give you information on a user at the time that user logs in, and asking it to give a list of _all_ users to every single Postgres instance you have on a regular timer. The latter is what seems to be problematic. > That's also why I specifically wasn't > suggesting that and was instead suggesting that we have something that's > connected to one of the (hopefully, many, many) LDAP servers and is > doing change monitoring, allowing changes to be pushed down to PG, > rather than cronjobs constantly running the same queries and re-checking > things over and over. I appreciate that that's also not free, but I > don't believe it's nearly as bad as the cron-based approach and it's > certainly something that an LDAP infrastructure should be really rather > good at. I guess I'd have to see an implementation -- I was under the impression that persistent search wasn't widely implemented? > > > All that said, I do see how having the ability to call out to another > > > system for mappings may be useful, so I'm not sure that we shouldn't > > > consider this specific change and have it be specifically just for > > > mappings, in which case pg_ident seems appropriate. > > > > Yeah, this PoC was mostly an increment on the functionality that > > already existed. The division between what goes in pg_hba and what goes > > in pg_ident is s
Re: PoC: using sampling to estimate joins / complex conditions
Hi, On 2021-06-27 19:55:24 +0200, Tomas Vondra wrote: > estimating joins is one of the significant gaps related to extended > statistics, and I've been regularly asked about what we might do about > that. This is an early experimental patch that I think might help us > with improving this, possible even in PG15. The tests in this patch fail: https://cirrus-ci.com/task/5304621299138560 https://api.cirrus-ci.com/v1/artifact/task/5304621299138560/regress_diffs/src/test/regress/regression.diffs Looks like the regression test output hasn't been updated? Greetings, Andres Freund
Re: In-placre persistance change of a relation
On 2021-12-23 15:33:35 +0900, Kyotaro Horiguchi wrote: > At Thu, 23 Dec 2021 15:01:41 +0900 (JST), Kyotaro Horiguchi > wrote in > > I added TAP test to excecise the in-place persistence change. > > We don't need a base table for every index. TAP test revised. The tap tests seems to fail on all platforms. See https://cirrus-ci.com/build/4911549314760704 E.g. the linux failure is [16:45:15.569] [16:45:15.569] # Failed test 'inserted' [16:45:15.569] # at t/027_persistence_change.pl line 121. [16:45:15.569] # Looks like you failed 1 test of 25. [16:45:15.569] [16:45:15] t/027_persistence_change.pl .. [16:45:15.569] Dubious, test returned 1 (wstat 256, 0x100) [16:45:15.569] Failed 1/25 subtests [16:45:15.569] [16:45:15] [16:45:15.569] [16:45:15.569] Test Summary Report [16:45:15.569] --- [16:45:15.569] t/027_persistence_change.pl(Wstat: 256 Tests: 25 Failed: 1) [16:45:15.569] Failed test: 18 [16:45:15.569] Non-zero exit status: 1 [16:45:15.569] Files=27, Tests=315, 220 wallclock secs ( 0.14 usr 0.03 sys + 48.94 cusr 17.13 csys = 66.24 CPU) https://api.cirrus-ci.com/v1/artifact/task/4785083130314752/tap/src/test/recovery/tmp_check/log/regress_log_027_persistence_change Greetings, Andres Freund
Re: biblio.sgml dead link
On Tue, Jan 04, 2022 at 06:10:07PM +0100, Erik Rijkers wrote: > The replacement is a ~200 euro pdf (2021). I'd be thankful if someone would > send the pdf to me; maybe I can update my JSON tests. > > And we should remove that entry from the bibliography (or have it point to > the new page [1]). Removing the entry seems a bit overdoing it to me, and updating to a paywall does not sound completely right to me either. Another thing that we could do is to just remove the link, but keep its reference. Any thoughts from others? -- Michael signature.asc Description: PGP signature
Re: Delay the variable initialization in get_rel_sync_entry
On Fri, Dec 24, 2021 at 01:27:26PM +, houzj.f...@fujitsu.com wrote: > Here is the perf result of pgoutput_change after applying the patch. > I didn't notice something else that stand out. > > |--2.99%--pgoutput_change > |--1.80%--get_rel_sync_entry > |--1.56%--hash_search > > Also attach complete profiles. Thanks. I have also done my own set of measurements, and the difference is noticeable in the profiles I looked at. So, applied down to 13. -- Michael signature.asc Description: PGP signature
RE: row filtering for logical replication
On Thu, Jan 4, 2022 at 00:54 PM Peter Smith wrote:
> Modified in v58 [1] as suggested
Thanks for updating the patches.
A few comments about v58-0001 and v58-0002.
v58-0001
1.
How about modifying the following loop in copy_table by using for_each_from
instead of foreach?
Like the invocation of for_each_from in function get_rule_expr.
from:
if (qual != NIL)
{
ListCell *lc;
boolfirst = true;
appendStringInfoString(&cmd, " WHERE ");
foreach(lc, qual)
{
char *q = strVal(lfirst(lc));
if (first)
first = false;
else
appendStringInfoString(&cmd, " OR ");
appendStringInfoString(&cmd, q);
}
list_free_deep(qual);
}
change to:
if (qual != NIL)
{
ListCell *lc;
char *q = strVal(linitial(qual));
appendStringInfo(&cmd, " WHERE %s", q);
for_each_from(lc, qual, 1)
{
q = strVal(lfirst(lc));
appendStringInfo(&cmd, " OR %s", q);
}
list_free_deep(qual);
}
2.
I find the API of get_rel_sync_entry is modified.
-get_rel_sync_entry(PGOutputData *data, Oid relid)
+get_rel_sync_entry(PGOutputData *data, Relation relation)
It looks like just moving the invocation of RelationGetRelid from outside into
function get_rel_sync_entry. I am not sure whether this modification is
necessary to this feature or not.
v58-0002
1.
In function pgoutput_row_filter_init, if no_filter is set, I think we do not
need to add row filter to list(rfnodes).
So how about changing three conditions when add row filter to rfnodes like this:
- if (pub->pubactions.pubinsert)
+ if (pub->pubactions.pubinsert &&
!no_filter[idx_ins])
{
rfnode =
stringToNode(TextDatumGetCString(rfdatum));
rfnodes[idx_ins] =
lappend(rfnodes[idx_ins], rfnode);
}
Regards,
Wang wei
Re: Converting WAL to SQL
On Tue, Jan 04, 2022 at 10:47:47AM -0300, Fabrízio de Royes Mello wrote: > I used it in the past during a major upgrade process from 9.2 to 9.6. > > What we did was decode the 9.6 wal files and apply transactions to the > old 9.2 to keep it in sync with the new promoted version. This was our > "rollback" strategy if something went wrong with the new 9.6 version. Oh, cool. Thanks for the feedback. -- Michael signature.asc Description: PGP signature
Re: GUC flags
At Tue, 28 Dec 2021 20:32:40 -0600, Justin Pryzby wrote in > On Thu, Dec 09, 2021 at 09:53:23AM -0600, Justin Pryzby wrote: > > On Thu, Dec 09, 2021 at 05:17:54PM +0900, Michael Paquier wrote: > > One option is to expose the GUC flags in pg_settings, so this can all be > > done > > in SQL regression tests. > > > > Maybe the flags should be text strings, so it's a nicer user-facing > > interface. > > But then the field would be pretty wide, even though we're only adding it > > for > > regression tests. The only other alternative I can think of is to make a > > sql-callable function like pg_get_guc_flags(text guc). > > Rebased on cab5b9ab2c066ba904f13de2681872dcda31e207. > > And added 0003, which changes to instead exposes the flags as a function, to > avoid changing pg_settings and exposing internally-defined integer flags in > that somewhat prominent view. Just an idea but couldn't we use flags in a series of one-letter flag representations? It is more user-friendly than integers but shorter than full-text representation. +SELECT name, flags FROM pg_settings; name | flags + application_name | ARsec transaction_deferrable | Arsec transaction_isolation | Arsec transaction_read_only | Arsec regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: [HACKERS] logical decoding of two-phase transactions
On Tue, Jan 4, 2022 at 9:00 AM Masahiko Sawada wrote: > > According to the doc, the two_phase field has: > > True if the slot is enabled for decoding prepared transactions. Always > false for physical slots. > > It's unnatural a bit to me that replication slots have such a property > since the replication slots have been used to protect WAL and tuples > that are required for logical decoding, physical replication, and > backup, etc from removal. Also, it seems that even if a replication > slot is created with two_phase = off, it's overwritten to on if the > plugin enables two-phase option. Is there any reason why we can turn > on and off this value on the replication slot side and is there any > use case where the replication slot’s two_phase is false and the > plugin’s two-phase option is on and vice versa? > We enable two_phase only when we start streaming from the subscriber-side. This is required because we can't enable it till the initial sync is complete, otherwise, it could lead to loss of data. See comments atop worker.c (description under the title: TWO_PHASE TRANSACTIONS). > I think that we can > have replication slots always have two_phase_at value and remove the > two_phase field from the view. > I am not sure how that will work because we can allow streaming of prepared transactions when the same is enabled at the CREATE SUBSCRIPTION time, the default for which is false. -- With Regards, Amit Kapila.
Re: Consider parallel for lateral subqueries with limit
On Tue, Jan 4, 2022 at 5:31 PM Tom Lane wrote: > > Greg Nancarrow writes: > > The patch LGTM. > > I have set the status to "Ready for Committer". > > I don't really see why this patch is even a little bit safe. > The argument for it seems to be that a lateral subquery will > necessarily be executed in such a way that each complete iteration > of the subquery, plus joining to its outer rel, happens within a > single worker ... but where is the guarantee of that? Once > you've marked the rel as parallel-safe, the planner is free to > consider all sorts of parallel join structures. I'm afraid this > would be easily broken as soon as you look at cases with three or > more rels. Or maybe even just two. The reason for the existing > restriction boils down to this structure being unsafe: > > Gather > NestLoop > Scan ... > Limit > Scan ... > > and I don't see how the existence of a lateral reference > makes it any safer. Thanks for taking a look. I'm not following how the structure you posited is inherently unsafe when it's a lateral reference. That limit/scan (if lateral) has to be being executed per tuple in the outer scan, right? And if it's a unique execution per tuple, then consistency across tuples (that are in different workers) can't be a concern. Is there a scenario I'm missing where lateral can currently be executed in that way in that structure (or a different one)? Thanks, James Coleman
Re: GUC flags
On Wed, Jan 05, 2022 at 11:47:57AM +0900, Kyotaro Horiguchi wrote: > At Tue, 28 Dec 2021 20:32:40 -0600, Justin Pryzby > wrote in > > On Thu, Dec 09, 2021 at 09:53:23AM -0600, Justin Pryzby wrote: > > > On Thu, Dec 09, 2021 at 05:17:54PM +0900, Michael Paquier wrote: > > > One option is to expose the GUC flags in pg_settings, so this can all be > > > done > > > in SQL regression tests. > > > > > > Maybe the flags should be text strings, so it's a nicer user-facing > > > interface. > > > But then the field would be pretty wide, even though we're only adding it > > > for > > > regression tests. The only other alternative I can think of is to make a > > > sql-callable function like pg_get_guc_flags(text guc). > > > > Rebased on cab5b9ab2c066ba904f13de2681872dcda31e207. > > > > And added 0003, which changes to instead exposes the flags as a function, to > > avoid changing pg_settings and exposing internally-defined integer flags in > > that somewhat prominent view. > > Just an idea but couldn't we use flags in a series of one-letter flag > representations? It is more user-friendly than integers but shorter > than full-text representation. > > +SELECT name, flags FROM pg_settings; > name | flags > + > application_name | ARsec > transaction_deferrable | Arsec > transaction_isolation | Arsec > transaction_read_only | Arsec It's a good idea. I suppose you intend that "A" means it's enabled and "a" means it's disabled ? A => show all R => reset all S => not in sample E => explain C => computed Which is enough to support the tests that I came up with: + (flags&4) != 0 AS no_show_all, + (flags&8) != 0 AS no_reset_all, + (flags&32) != 0 AS not_in_sample, + (flags&1048576) != 0 AS guc_explain, + (flags&2097152) != 0 AS guc_computed However, I think if we add a field to pg_stat_activity, it would be in a separate patch, expected to be independently useful. 1) expose GUC flags to pg_stat_activity; 2) rewrite check_guc as a sql regression test; In that case, *all* the flags should be exposed. There's currently 20, which means it may not work well after all - it's already too long, and could get longer, and/or overflow the alphabet... I think pg_get_guc_flags() may be best, but I'm interested to hear other opinions. -- Justin
Re: Suggestion: optionally return default value instead of error on failed cast
>
> currently a failed cast throws an error. It would be useful to have a
> way to get a default value instead.
>
I've recently encountered situations where this would have been helpful.
Recently I came across some client code:
CREATE OR REPLACE FUNCTION is_valid_json(str text) RETURNS boolean LANGUAGE
PLPGSQL
AS $$
DECLARE
j json;
BEGIN
j := str::json;
return true;
EXCEPTION WHEN OTHERS THEN return false;
END
$$;
This is a double-bummer. First, the function discards the value so we have
to recompute it, and secondly, the exception block prevents the query from
being parallelized.
>
> T-SQL has try_cast [1]
>
I'd be more in favor of this if we learn that there's no work (current or
proposed) in the SQL standard.
> Oracle has CAST(... AS .. DEFAULT ... ON CONVERSION ERROR) [2]
>
If the SQL group has suggested anything, I'd bet it looks a lot like this.
>
> The DEFAULT ... ON CONVERSION ERROR syntax seems like it could be
> implemented in PostgreSQL. Even if only DEFAULT NULL was supported (at
> first) that would already help.
>
> The short syntax could be extended for the DEFAULT NULL case, too:
>
> SELECT '...'::type -- throws error
> SELECT '...':::type -- returns NULL
>
I think I'm against adding a ::: operator, because too many people are
going to type (or omit) the third : by accident, and that would be a really
subtle bug. The CAST/TRY_CAST syntax is wordy but it makes it very clear
that you expected janky input and have specified a contingency plan.
The TypeCast node seems like it wouldn't need too much modification to
allow for this. The big lift, from what I can tell, is either creating
versions of every $foo_in() function to return NULL instead of raising an
error, and then effectively wrapping that inside a coalesce() to process
the default. Alternatively, we could add an extra boolean parameter
("nullOnFailure"? "suppressErrors"?) to the existing $foo_in() functions, a
boolean to return null instead of raising an error, and the default would
be handled in coerce_to_target_type(). Either of those would create a fair
amount of work for extensions that add types, but I think the value would
be worth it.
I do remember when I proposed the "void"/"black hole"/"meh" datatype (all
values map to NULL) I ran into a fairly fundamental rule that types must
map any not-null input to a not-null output, and this could potentially
violate that, but I'm not sure.
Does anyone know if the SQL standard has anything to say on this subject?
Re: [PoC] Delegating pg_ident to a third party
Greetings, On Tue, Jan 4, 2022 at 18:56 Jacob Champion wrote: > On Mon, 2022-01-03 at 19:42 -0500, Stephen Frost wrote: > > * Jacob Champion (pchamp...@vmware.com) wrote: > > > > > > That last point was my motivation for the authn_id patch [1] -- so that > > > auditing could see the actual user _and_ the generic role. The > > > information is already there to be used, it's just not exposed to the > > > stats framework yet. > > > > While that helps, and I generally support adding that information to the > > logs, it's certainly not nearly as good or useful as having the actual > > user known to the database. > > Could you talk more about the use cases for which having the "actual > user" is better? From an auditing perspective I don't see why > "authenticated as ja...@example.net, logged in as admin" is any worse > than "logged in as jacob". The above case isn’t what we are talking about, as far as I understand anyway. You’re suggesting “authenticated as ja...@example.net, logged in as sales” where the user in the database is “sales”. Consider triggers which only have access to “sales”, or a tool like pgaudit which only has access to “sales”. Who was it in sales that updated that record though? We don’t know- we would have to go try to figure it out from the logs, but even if we had time stamps on the row update, there could be 50 sales people logged in at overlapping times. > > Forcing one role per individual end user is wasteful and isn't really > > > making good use of the role-based system that you already have. > > > Generally speaking, when administering hundreds or thousands of users, > > > people start dividing them up into groups as opposed to dealing with > > > them individually. So I don't think new features should be taking away > > > flexibility in this area -- if one role per user already works well for > > > you, great, but don't make everyone do the same. > > > > Using the role system we have to assign privileges certainly is useful > > and sensible, of course, though I don't see where you've actually made > > an argument for why one role per individual is somehow wasteful or > > somehow takes away from the role system that we have for granting > > rights. > > I was responding more to your statement that "Being able to have roles > and memberships automatically created is much more the direction that > I'd say we should be going in". It's not that one-role-per-user is > inherently wasteful, but forcing role proliferation where it's not > needed is. If all users have the same set of permissions, there doesn't > need to be more than one role. But see below. Just saying it’s wasteful isn’t actually saying what is wasteful about it. > I'm also not suggesting that we make everyone do the same > > thing, indeed, later on I was supportive of having an external system > > provide the mapping. Here, I'm just making the point that we should > > also be looking at automatic role/membership creation. > > Gotcha. Agreed; that would open up the ability to administer role > privileges externally too, which would be cool. That could be used in > tandem with something like this patchset. Not sure exactly what you’re referring to here by “administer role privileges externally too”..? Curious to hear what you are imagining specifically. > > > I'd go a step further and suggest that the way to do this is with a > > > > background worker that's started up and connects to an LDAP > > > > infrastructure and listens for changes, allowing the system to pick > up > > > > on new roles/memberships as soon as they're created in the LDAP > > > > environment. That would then be controlled by appropriate settings > in > > > > postgresql.conf/.auto.conf. > > > > > > This is roughly what you can already do with existing (third-party) > > > tools, and that approach isn't scaling out in practice for some of our > > > existing customers. The load on the central server, for thousands of > > > idle databases dialing in just to see if there are any new users, is > > > huge. > > > > If you're referring specifically to cron-based tools which are > > constantly hammering on the LDAP servers running the same queries over > > and over, sure, I agree that that's creating load on the LDAP > > infrastructure (though, well, it was kind of designed to be very > > scalable for exactly that kind of load, no? So I'm not really sure why > > that's such an issue..). > > I don't have hands-on experience here -- just going on what I've been > told via field/product teams -- but it seems to me that there's a big > difference between asking an LDAP server to give you information on a > user at the time that user logs in, and asking it to give a list of > _all_ users to every single Postgres instance you have on a regular > timer. The latter is what seems to be problematic. And to be clear, I agree that’s not good (though, again, really, your ldap infrastructure shouldn’t be having all that much trouble with it- you can scale thos
Re: Skipping logical replication transactions on subscriber side
On Mon, Dec 27, 2021 at 9:54 AM Masahiko Sawada wrote:
>
> On Thu, Dec 16, 2021 at 2:42 PM Masahiko Sawada wrote:
> >
> > On Thu, Dec 16, 2021 at 2:21 PM Amit Kapila wrote:
> > >
> > > On Thu, Dec 16, 2021 at 10:37 AM Masahiko Sawada
> > > wrote:
> > > >
> > > > On Thu, Dec 16, 2021 at 11:43 AM Amit Kapila
> > > > wrote:
> > > > >
> > > > > I thought we just want to lock before clearing the skip_xid something
> > > > > like take the lock, check if the skip_xid in the catalog is the same
> > > > > as we have skipped, if it is the same then clear it, otherwise, leave
> > > > > it as it is. How will that disallow users to change skip_xid when we
> > > > > are skipping changes?
> > > >
> > > > Oh I thought we wanted to keep holding the lock while skipping changes
> > > > (changing skip_xid requires acquiring the lock).
> > > >
> > > > So if skip_xid is already changed, the apply worker would do
> > > > replorigin_advance() with WAL logging, instead of committing the
> > > > catalog change?
> > > >
> > >
> > > Right. BTW, how are you planning to advance the origin? Normally, a
> > > commit transaction would do it but when we are skipping all changes,
> > > the commit might not do it as there won't be any transaction id
> > > assigned.
> >
> > I've not tested it yet but replorigin_advance() with wal_log = true
> > seems to work for this case.
>
> I've tested it and realized that we cannot use replorigin_advance()
> for this purpose without changes. That is, the current
> replorigin_advance() doesn't allow to advance the origin by the owner:
>
> /* Make sure it's not used by somebody else */
> if (replication_state->acquired_by != 0)
> {
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_IN_USE),
> errmsg("replication origin with OID %d is already
> active for PID %d",
> replication_state->roident,
> replication_state->acquired_by)));
> }
>
> So we need to change it so that the origin owner can advance its
> origin, which makes sense to me.
>
> Also, when we have to update the origin instead of committing the
> catalog change while updating the origin, we cannot record the origin
> timestamp.
>
Is it because we currently update the origin timestamp with commit record?
> This behavior makes sense to me because we skipped the
> transaction. But ISTM it’s not good if we emit the origin timestamp
> only when directly updating the origin. So probably we need to always
> omit origin timestamp.
>
Do you mean to say that you want to omit it even when we are
committing the changes?
> Apart from that, I'm vaguely concerned that the logic seems to be
> getting complex. Probably it comes from the fact that we store
> skip_xid in the catalog and update the catalog to clear/set the
> skip_xid. It might be worth revisiting the idea of storing skip_xid on
> shmem (e.g., ReplicationState)?
>
IIRC, the problem with that idea was that we won't remember skip_xid
information after server restart and the user won't even know that it
has to set it again.
--
With Regards,
Amit Kapila.
Re: Skipping logical replication transactions on subscriber side
On Wed, Jan 5, 2022 at 9:01 AM Amit Kapila wrote: > > On Mon, Dec 27, 2021 at 9:54 AM Masahiko Sawada wrote: > > Do you mean to say that you want to omit it even when we are > committing the changes? > > > Apart from that, I'm vaguely concerned that the logic seems to be > > getting complex. Probably it comes from the fact that we store > > skip_xid in the catalog and update the catalog to clear/set the > > skip_xid. It might be worth revisiting the idea of storing skip_xid on > > shmem (e.g., ReplicationState)? > > > > IIRC, the problem with that idea was that we won't remember skip_xid > information after server restart and the user won't even know that it > has to set it again. I agree, that if we don't keep it in the catalog then after restart if the transaction replayed again then the user has to set the skip xid again and that would be pretty inconvenient because the user might have to analyze the failure again and repeat the same process he did before restart. But OTOH the combination of restart and the skip xid might not be very frequent so this might not be a very bad option. Basically, I am in favor of storing it in a catalog as that solution looks cleaner at least from the user pov but if we think there are a lot of complexities from the implementation pov then we might analyze the approach of storing in shmem as well. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: pg_stat_statements and "IN" conditions
On 1/5/22 4:02 AM, Tom Lane wrote: Dmitry Dolgov <9erthali...@gmail.com> writes: And now for something completely different, here is a new patch version. It contains a small fix for one problem we've found during testing (one path code was incorrectly assuming find_const_walker results). I've been saying from day one that pushing the query-hashing code into the core was a bad idea, and I think this patch perfectly illustrates why. We can debate whether the rules proposed here are good for pg_stat_statements or not, but it seems inevitable that they will be a disaster for some other consumers of the query hash. In particular, dropping external parameters from the hash seems certain to break something for somebody +1. In a couple of extensions I use different logic of query jumbling - hash value is more stable in some cases than in default implementation. For example, it should be stable to permutations in 'FROM' section of a query. And If anyone subtly changes jumbling logic when the extension is active, the instance could get huge performance issues. Let me suggest, that the core should allow an extension at least to detect such interference between extensions. Maybe hook could be replaced with callback to allow extension see an queryid with underlying generation logic what it expects. -- regards, Andrey Lepikhov Postgres Professional
Emit "checkpoint skipped because system is idle" message at LOG level if log_checkpoints is set
Hi, Postgres server emits a message at DEBUG1 level when it skips a checkpoint. At times, developers might be surprised after figuring out from server logs that there were no checkpoints happening at all during a certain period of time when DEBUG1 messages aren't captured. How about emitting the message at LOG level if log_checkpoints is set? Patch attached. Thoughts? Regards, Bharath Rupireddy. v1-0001-Emit-checkpoint-skipped-message-at-LOG-level.patch Description: Binary data
Re: Index-only scan for btree_gist turns bpchar to char
04.01.2022 22:19, Tom Lane wrote: > Alexander Lakhin writes: >> While testing the index-only scan fix, I've discovered that replacing >> the index-only scan with the index scan changes contrib/btree_gist >> output because index-only scan for btree_gist returns a string without >> padding. > Ugh, yeah. This seems to be because gbt_bpchar_compress() strips > trailing spaces (using rtrim1) before storing the value. The > idea evidently is to simplify gbt_bpchar_consistent, but it's not > acceptable if the opclass is supposed to support index-only scan. > > I see two ways to fix this: > > * Disallow index-only scan, by removing the fetch function for this > opclass. This'd require a module version bump, so people wouldn't > get that fix automatically. > > * Change gbt_bpchar_compress to not trim spaces (it becomes just > like gbt_text_compress), and adapt gbt_bpchar_consistent to cope. > This does nothing for the problem immediately, unless you REINDEX > affected indexes --- but over time an index's entries would get > replaced with untrimmed versions. I think that the second way is preferable in the long run. It doesn't need an explanation after years, why index-only scan is not supported for that type. One-time mentioning the change and the need for REINDEX in release notes seems more future-oriented to me. Best regards, Alexander
Re: pg_stat_statements and "IN" conditions
"Andrey V. Lepikhov" writes: > On 1/5/22 4:02 AM, Tom Lane wrote: >> I've been saying from day one that pushing the query-hashing code into the >> core was a bad idea, and I think this patch perfectly illustrates why. > +1. > Let me suggest, that the core should allow an extension at least to > detect such interference between extensions. Maybe hook could be > replaced with callback to allow extension see an queryid with underlying > generation logic what it expects. I feel like we need to get away from the idea that there is just one query hash, and somehow let different extensions attach differently-calculated hashes to a query. I don't have any immediate ideas about how to do that in a reasonably inexpensive way. regards, tom lane
Re: Emit "checkpoint skipped because system is idle" message at LOG level if log_checkpoints is set
On Wed, Jan 5, 2022 at 10:24 AM Bharath Rupireddy wrote: > > Hi, > > Postgres server emits a message at DEBUG1 level when it skips a > checkpoint. At times, developers might be surprised after figuring out > from server logs that there were no checkpoints happening at all > during a certain period of time when DEBUG1 messages aren't captured. > How about emitting the message at LOG level if log_checkpoints is set? > Patch attached. > > Thoughts? +1 to convert to LOG when log_checkpoints is set. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: GUC flags
On Tue, Jan 04, 2022 at 09:06:48PM -0600, Justin Pryzby wrote: > I think pg_get_guc_flags() may be best, but I'm interested to hear other > opinions. My opinion on this matter is rather close to what you have here with handling things through one extra attribute. But I don't see the point of using an extra function where users would need to do a manual mapping of the flag bits back to a a text representation of them. So I would suggest to just add one text[] to pg_show_all_settings, with values being the bit names themselves, without the prefix "GUC_", for the ones we care most about. Sticking with one column for each one would require a catversion bump all the time, which could be cumbersome in the long run. -- Michael signature.asc Description: PGP signature
Re: row filtering for logical replication
On Tue, Jan 4, 2022 at 12:15 PM Peter Smith wrote: > > On Fri, Dec 31, 2021 at 12:39 AM houzj.f...@fujitsu.com > wrote: > > > 3) v55-0002 > > > +static bool pgoutput_row_filter_update_check(enum > > > ReorderBufferChangeType changetype, Relation relation, > > > + > > >HeapTuple oldtuple, HeapTuple newtuple, > > > + > > >RelationSyncEntry *entry, ReorderBufferChangeType *action); > > > > > > Do we need parameter changetype here? I think it could only be > > > REORDER_BUFFER_CHANGE_UPDATE. > > > > I didn't change this, I think it might be better to wait for Ajin's opinion. > > I agree with Tang. AFAIK there is no problem removing that redundant > param as suggested. BTW - the Assert within that function is also > incorrect because the only possible value is > REORDER_BUFFER_CHANGE_UPDATE. I will make these fixes in a future > version. > That sounds fine to me too. One more thing is that you don't need to modify the action in case it remains update as the caller has already set that value. Currently, we are modifying it as update at two places in this function, we can remove both of those and keep the comments intact for the later update. -- With Regards, Amit Kapila.
Re: row filtering for logical replication
I have reviewed again the source code for v58-0001.
Below are my review comments.
Actually, I intend to fix most of these myself for v59*, so this post
is just for records.
v58-0001 Review Comments
1. doc/src/sgml/ref/alter_publication.sgml - reword for consistency
+ name to explicitly indicate that descendant tables are included. If the
+ optional WHERE clause is specified, rows that do not
+ satisfy the expression will
+ not be published. Note that parentheses are required around the
For consistency, it would be better to reword this sentence about the
expression to be more similar to the one in CREATE PUBLICATION, which
now says:
+ If the optional WHERE clause is specified, rows for
+ which the expression returns
+ false or null will not be published. Note that parentheses are required
+ around the expression. It has no effect on TRUNCATE
+ commands.
~~
2. doc/src/sgml/ref/create_subscription.sgml - reword for consistency
@@ -319,6 +324,25 @@ CREATE SUBSCRIPTION subscription_namecreate_slot = false. This is an
implementation restriction that might be lifted in a future release.
+
+
+ If any table in the publication has a WHERE clause, rows
+ that do not satisfy the expression
+ will not be published. If the subscription has several publications in which
For consistency, it would be better to reword this sentence about the
expression to be more similar to the one in CREATE PUBLICATION, which
now says:
+ If the optional WHERE clause is specified, rows for
+ which the expression returns
+ false or null will not be published. Note that parentheses are required
+ around the expression. It has no effect on TRUNCATE
+ commands.
~~
3. src/backend/catalog/pg_publication.c - whitespace
+rowfilter_walker(Node *node, Relation relation)
+{
+ char*errdetail_msg = NULL;
+
+ if (node == NULL)
+ return false;
+
+
+ if (IsRowFilterSimpleExpr(node))
Remove the extra blank line.
~~
4. src/backend/executor/execReplication.c - move code
+ bad_rfcolnum = GetRelationPublicationInfo(rel, true);
+
+ /*
+ * It is only safe to execute UPDATE/DELETE when all columns referenced in
+ * the row filters from publications which the relation is in are valid,
+ * which means all referenced columns are part of REPLICA IDENTITY, or the
+ * table do not publish UPDATES or DELETES.
+ */
+ if (AttributeNumberIsValid(bad_rfcolnum))
I felt that the bad_rfcolnum assignment belongs below the large
comment explaining this logic.
~~
5. src/backend/executor/execReplication.c - fix typo
+ /*
+ * It is only safe to execute UPDATE/DELETE when all columns referenced in
+ * the row filters from publications which the relation is in are valid,
+ * which means all referenced columns are part of REPLICA IDENTITY, or the
+ * table do not publish UPDATES or DELETES.
+ */
Typo: "table do not publish" -> "table does not publish"
~~
6. src/backend/replication/pgoutput/pgoutput.c - fix typo
+ oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+ /* Gather the rfnodes per pubaction of this publiaction. */
+ if (pub->pubactions.pubinsert)
Typo: "publiaction" --> "publication"
~~
7. src/backend/utils/cache/relcache.c - fix comment case
@@ -267,6 +271,19 @@ typedef struct opclasscacheent
static HTAB *OpClassCache = NULL;
+/*
+ * Information used to validate the columns in the row filter expression. see
+ * rowfilter_column_walker for details.
+ */
Typo: "see" --> "See"
~~
8. src/backend/utils/cache/relcache.c - "row-filter"
For consistency with all other naming change all instances of
"row-filter" to "row filter" in this file.
~~
9. src/backend/utils/cache/relcache.c - fix typo
~~
10. src/backend/utils/cache/relcache.c - comment confused wording?
Function GetRelationPublicationInfo:
+ /*
+ * For a partition, if pubviaroot is true, check if any of the
+ * ancestors are published. If so, note down the topmost ancestor
+ * that is published via this publication, the row filter
+ * expression on which will be used to filter the partition's
+ * changes. We could have got the topmost ancestor when collecting
+ * the publication oids, but that will make the code more
+ * complicated.
+ */
Typo: Probably "on which' --> "of which" ?
~~
11. src/backend/utils/cache/relcache.c - GetRelationPublicationActions
Something seemed slightly fishy with the code doing the memcpy,
because IIUC is possible for the GetRelationPublicationInfo function
to return without setting the relation->rd_pubactions. Is it just
missing an Assert or maybe a comment to say such a scenario is not
possible in this case because the is_publishable_relation was already
tested?
Currently, it just seems a little bit too sneaky.
~~
12. src/include/parser/parse_node.h - This change is unrelated to row-filtering.
@@ -79,7 +79,7 @@ typedef enum ParseExprKind
EXPR_KIND_CALL_ARGUMENT, /* procedure argument in CALL */
EXPR_KIND_COPY_WHERE, /* WHERE co
Re: row filtering for logical replication
On Wed, Jan 5, 2022 at 4:34 PM Peter Smith wrote: > > I have reviewed again the source code for v58-0001. > > Below are my review comments. > > Actually, I intend to fix most of these myself for v59*, so this post > is just for records. > > v58-0001 Review Comments > > > ~~ > > 9. src/backend/utils/cache/relcache.c - fix typo > (Oops. The previous post omitted the detail for this comment #9) - * If we know everything is replicated, there is no point to check for - * other publications. + * If the publication action include UPDATE and DELETE and + * validate_rowfilter flag is true, validates that any columns + * referenced in the filter expression are part of REPLICA IDENTITY + * index. Typo: "If the publication action include UPDATE and DELETE" --> "If the publication action includes UPDATE or DELETE" -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Add 64-bit XIDs into PostgreSQL 15
On 2021/12/30 21:15, Maxim Orlov wrote: Hi, hackers! Long time wraparound was a really big pain for highly loaded systems. One source of performance degradation is the need to vacuum before every wraparound. And there were several proposals to make XIDs 64-bit like [1], [2], [3] and [4] to name a few. The approach [2] seems to have stalled on CF since 2018. But meanwhile it was successfully being used in our Postgres Pro fork all time since then. We have hundreds of customers using 64-bit XIDs. Dozens of instances are under load that require wraparound each 1-5 days with 32-bit XIDs. It really helps the customers with a huge transaction load that in the case of 32-bit XIDs could experience wraparounds every day. So I'd like to propose this approach modification to CF. PFA updated working patch v6 for PG15 development cycle. It is based on a patch by Alexander Korotkov version 5 [5] with a few fixes, refactoring and was rebased to PG15. Thanks a lot! I'm really happy to see this proposal again!! Is there any documentation or README explaining this whole 64-bit XID mechanism? Could you tell me what happens if new tuple with XID larger than xid_base + 0x is inserted into the page? Such new tuple is not allowed to be inserted into that page? Or xid_base and xids of all existing tuples in the page are increased? Also what happens if one of those xids (of existing tuples) cannot be changed because the tuple still can be seen by very-long-running transaction? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Delay the variable initialization in get_rel_sync_entry
On Wednesday, January 5, 2022 9:31 AM Michael Paquier wrote: > On Fri, Dec 24, 2021 at 01:27:26PM +, houzj.f...@fujitsu.com wrote: > > Here is the perf result of pgoutput_change after applying the patch. > > I didn't notice something else that stand out. > > > > |--2.99%--pgoutput_change > > |--1.80%--get_rel_sync_entry > > |--1.56%--hash_search > > > > Also attach complete profiles. > > Thanks. I have also done my own set of measurements, and the difference is > noticeable in the profiles I looked at. So, applied down to 13. Thanks for pushing! Best regards, Hou zj
Re: SQL/JSON: functions
On Thu, Dec 9, 2021 at 7:34 PM Himanshu Upadhyaya
wrote:
> 3)
> Is not that result of the two below queries should match because both are
> trying to retrieve the information from the JSON object.
>
> postgres=# SELECT JSON_OBJECT('track' VALUE '{
> "segments": [
> {
> "location": [ 47.763, 13.4034 ],
> "start time": "2018-10-14 10:05:14",
> "HR": 73
> },
> {
> "location": [ 47.706, 13.2635 ],
> "start time": "2018-10-14 101:39:21",
> "HR": 135
> }
> ]
> }
> }')->'track'->'segments';
> ?column?
> --
>
> (1 row)
>
> postgres=# select '{
> "track": {
> "segments": [
> {
> "location": [ 47.763, 13.4034 ],
> "start time": "2018-10-14 10:05:14",
> "HR": 73
> },
> {
> "location": [ 47.706, 13.2635 ],
> "start time": "2018-10-14 10:39:21",
> "HR": 135
> }
> ]
> }
> }'::jsonb->'track'->'segments';
>
> ?column?
> ---
> [{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14
> 10:05:14"}, {"HR": 135, "location": [47.706, 13.2635], "start time":
> "2018-10-14 10:39:21"}]
> (1 row)
>
just wanted to check your opinion on the above, is this an expected behaviour?
> Few comments For 0002-SQL-JSON-constructors-v59.patch:
Also, any thoughts on this?
--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
Re: row filtering for logical replication
On Wed, Dec 22, 2021 at 5:26 AM Peter Smith wrote:
>
> On Mon, Dec 20, 2021 at 9:30 PM Amit Kapila wrote:
> >
> > On Mon, Dec 20, 2021 at 8:41 AM houzj.f...@fujitsu.com
> > wrote:
> > >
> > > Thanks for the comments, I agree with all the comments.
> > > Attach the V49 patch set, which addressed all the above comments on the
> > > 0002
> > > patch.
> > >
> >
> > Few comments/suugestions:
> > ==
> > 1.
> > + Oid publish_as_relid = InvalidOid;
> > +
> > + /*
> > + * For a partition, if pubviaroot is true, check if any of the
> > + * ancestors are published. If so, note down the topmost ancestor
> > + * that is published via this publication, the row filter
> > + * expression on which will be used to filter the partition's
> > + * changes. We could have got the topmost ancestor when collecting
> > + * the publication oids, but that will make the code more
> > + * complicated.
> > + */
> > + if (pubform->pubviaroot && relation->rd_rel->relispartition)
> > + {
> > + if (pubform->puballtables)
> > + publish_as_relid = llast_oid(ancestors);
> > + else
> > + publish_as_relid = GetTopMostAncestorInPublication(pubform->oid,
> > +ancestors);
> > + }
> > +
> > + if (publish_as_relid == InvalidOid)
> > + publish_as_relid = relid;
> >
> > I think you can initialize publish_as_relid as relid and then later
> > override it if required. That will save the additional check of
> > publish_as_relid.
> >
>
> Fixed in v51* [1]
>
> > 2. I think your previous version code in GetRelationPublicationActions
> > was better as now we have to call memcpy at two places.
> >
>
> Fixed in v51* [1]
>
> > 3.
> > +
> > + if (list_member_oid(GetRelationPublications(ancestor),
> > + puboid) ||
> > + list_member_oid(GetSchemaPublications(get_rel_namespace(ancestor)),
> > + puboid))
> > + {
> > + topmost_relid = ancestor;
> > + }
> >
> > I think here we don't need to use braces ({}) as there is just a
> > single statement in the condition.
> >
>
> Fixed in v51* [1]
>
> > 4.
> > +#define IDX_PUBACTION_n 3
> > + ExprState*exprstate[IDX_PUBACTION_n]; /* ExprState array for row
> > filter.
> > +One per publication action. */
> > ..
> > ..
> >
> > I think we can have this define outside the structure. I don't like
> > this define name, can we name it NUM_ROWFILTER_TYPES or something like
> > that?
> >
>
> Partly fixed in v51* [1], I've changed the #define name but I did not
> move it. The adjacent comment talks about these ExprState caches and
> explains the reason why the number is 3. So if I move the #define then
> half that comment would have to move with it. I thought it is better
> to keep all the related parts grouped together with the one
> explanatory comment, but if you still want the #define moved please
> confirm and I can do it in a future version.
>
Yeah, I would prefer it to be moved. You can move the part of the
comment suggesting three pubactions can be used for row filtering.
--
With Regards,
Amit Kapila.
Re: row filtering for logical replication
On Tue, Jan 4, 2022 at 9:58 AM Peter Smith wrote:
>
> Here is the v58* patch set:
>
> Main changes from v57* are
> 1. Couple of review comments fixed
>
> ~~
>
> Review comments (details)
> =
>
> v58-0001 (main)
> - PG docs updated as suggested [Alvaro, Euler 26/12]
>
> v58-0002 (new/old tuple)
> - pgputput_row_filter_init refactored as suggested [Wangw 30/12] #3
> - re-ran pgindent
>
> v58-0003 (tab, dump)
> - no change
>
> v58-0004 (refactor transformations)
> - minor changes to commit message
Few comments:
1) We could include namespace names along with the relation to make it
more clear to the user if the user had specified tables having same
table names from different schemas:
+ /* Disallow duplicate tables if there are any
with row filters. */
+ if (t->whereClause ||
list_member_oid(relids_with_rf, myrelid))
+ ereport(ERROR,
+
(errcode(ERRCODE_DUPLICATE_OBJECT),
+errmsg("conflicting
or redundant WHERE clauses for table \"%s\"",
+
RelationGetRelationName(rel;
2) Few includes are not required, I could compile without it:
#include "executor/executor.h" in pgoutput.c,
#include "parser/parse_clause.h",
#include "parser/parse_relation.h" and #include "utils/ruleutils.h" in
relcache.c and
#include "parser/parse_node.h" in pg_publication.h
3) I felt the 0004-Row-Filter-refactor-transformations can be merged
to 0001 patch, since most of the changes are from 0001 patch or the
functions which are moved from pg_publication.c to publicationcmds.c
can be handled in 0001 patch.
4) Should this be posted as a separate patch in a new thread, as it is
not part of row filtering:
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -79,7 +79,7 @@ typedef enum ParseExprKind
EXPR_KIND_CALL_ARGUMENT,/* procedure argument in CALL */
EXPR_KIND_COPY_WHERE, /* WHERE condition in COPY FROM */
EXPR_KIND_GENERATED_COLUMN, /* generation expression for a column */
- EXPR_KIND_CYCLE_MARK, /* cycle mark value */
+ EXPR_KIND_CYCLE_MARK/* cycle mark value */
} ParseExprKind;
5) This log will be logged for each tuple, if there are millions of
records it will get logged millions of times, we could remove it:
+ /* update requires a new tuple */
+ Assert(newtuple);
+
+ elog(DEBUG3, "table \"%s.%s\" has row filter",
+
get_namespace_name(get_rel_namespace(RelationGetRelid(relation))),
+get_rel_name(relation->rd_id));
Regards,
Vignesh
Re: [Proposal] Add foreign-server health checks infrastructure
Thank you for the new patch! On 2021-12-15 15:40, kuroda.hay...@fujitsu.com wrote: Dear Kato-san, Thank you for giving comments! And sorry for late reply. I rebased my patches. Even for local-only transaction, I thought it useless to execute CallCheckingRemoteServersCallbacks() and enable_timeout_after(). Can I make it so that it determines outside whether it contains SQL to the remote or not? Yeah, remote-checking timeout will be enable even ifa local transaction is opened. In my understanding, postgres cannot distinguish whether opening transactions are using only local object or not. My first idea was that turning on the timeout when GetFdwRoutineXXX functions were called, but in some cases FDWs may not use remote connection even if they call such a handler function. e.g. postgresExplainForeignScan(). Hence I tried another approach that unregister all checking callback the end of each transactions. Only FDWs can notice that transactions are remote or local, so they must register callback when they really connect to other database. This implementation will avoid calling remote checking in the case of local transaction, but multiple registering and unregistering may lead another overhead. I attached which implements that. It certainly incurs another overhead, but I think it's better than the previous one. So far, I haven't encountered any problems, but I'd like other people's opinions. The following points are minor. 1. In foreign.c, some of the lines are too long and should be broken. 2. In pqcomm.c, the newline have been removed, what is the intention of this? 3. In postgres.c, 3-1. how about inserting a comment between lines 2713 and 2714, similar to line 2707? 3-2. the line breaks in line 3242 and line 3243 should be aligned. 3-3. you should change /* * Skip checking foreign servers while reading messages. */ to /* * Skip checking foreign servers while reading messages. */ 4. In connection.c, There is a typo in line 1684, so "fucntion" should be changed to "function". Maybe all of them were fixed. Thanks! Thank you, and it looks good to me. -- Regards, -- Shinya Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Is it worth adding ReplicationSlot active_pid to ReplicationSlotPersistentData?
At Tue, 28 Dec 2021 12:28:07 +0530, Bharath Rupireddy
wrote in
> On Wed, Dec 15, 2021 at 8:32 AM Kyotaro Horiguchi
> wrote:
> > > Here's the patch that adds a LOG message whenever a replication slot
> > > becomes active and inactive. These logs will be extremely useful on
> > > production servers to debug and analyze inactive replication slot
> > > issues.
> > >
> > > Thoughts?
> >
> > If I create a replication slot, I saw the following lines in server log.
> >
> > [22054:client backend] LOG: replication slot "s1" becomes active
> > [22054:client backend] DETAIL: The process with PID 22054 acquired it.
> > [22054:client backend] STATEMENT: select pg_drop_replication_slot('s1');
> > [22054:client backend] LOG: replication slot "s1" becomes inactive
> > [22054:client backend] DETAIL: The process with PID 22054 released it.
> > [22054:client backend] STATEMENT: select pg_drop_replication_slot('s1');
> >
> > They are apparently too much as if they were DEBUG3 lines. The
> > process PID shown is of the process the slot operations took place so
> > I think it conveys no information. The STATEMENT lines are also noisy
> > for non-ERROR emssages. Couldn't we hide that line?
> >
> > That is, how about making the log lines as simple as the follows?
> >
> > [17156:walsender] LOG: acquired replication slot "s1"
> > [17156:walsender] LOG: released replication slot "s1"
>
> Thanks for taking a look at the patch. Here's what I've come up with:
>
> for drops:
(two log lines per slot: acquire->drop)
>
> for creates:
(two log lines per slot: create->release)
..
Theses are still needlessly verbose. Even for those who want slot
activities to be logged are not interested in this detail. This is
still debug logs in that sense.
> The amount of logs may seem noisy, but they do help a lot given the
> fact that the server generates much more noise, for instance, the
> server logs the syntax error statements. And, the replication slots
> don't get created/dropped every now and then (at max, the
> pg_basebackup if at all used and configured to take the backups, say,
> every 24hrs or so). With the above set of logs debugging for inactive
In a nearby thread, there was a discussion that checkpoint logs are
too noisy to defaultly turn on. Finally it is committed but I don't
think this is committed as is as it is more verbose (IMV) than the
checkpoint logs. Thus this logs need to be muteable. Meanwhile I
don't think we willingly add a new knob for this feature. I think we
can piggy-back on log_replication_commands for the purpose, changing
its meaning slightly to "log replication commands and related
activity".
> replication slots becomes easier. One can find the root cause and
> answer questions like "why there was a huge WAL file growth at some
> point or when did a replication slot become inactive or how much time
> a replication slot was inactive or when did a standby disconnected and
> connected again or when did a pg_receivewal or pg_recvlogical
> connected and disconnected so on.".
I don't deny it is useful in that cases. If you are fine that the
logs are debug-only, making them DEBUG1 would work. But I don't think
you are fine with that since I think you are going to turn on them on
production systems.
> Here's the v2 patch. Please provide your thoughts.
Thanks! I have three comments on this version.
- I still think "acquire/release" logs on creation/dropping should be
silenced. Adding the third parameter to ReplicationSlotAcquire()
that can mute the acquiring (and as well as the corresponding
releasing) log will work.
- Need to mute the logs by log_replication_commands. (We could add
another choice for the variable for this purpose but I think we
don't need it.)
- The messages are not translatable as the variable parts are
adjectives. They need to consist of static sentences. The
combinations of the two properties are 6 (note that persistence is
tristate) but I don't think we accept that complexity for the
information. So I recommend to just remove the variable parts from
the messages.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: row filtering for logical replication
On Wed, Jan 5, 2022 at 11:04 AM Peter Smith wrote: > > > 11. src/backend/utils/cache/relcache.c - GetRelationPublicationActions > > Something seemed slightly fishy with the code doing the memcpy, > because IIUC is possible for the GetRelationPublicationInfo function > to return without setting the relation->rd_pubactions. Is it just > missing an Assert or maybe a comment to say such a scenario is not > possible in this case because the is_publishable_relation was already > tested? > I think it would be good to have an Assert for a valid value of relation->rd_pubactions before doing memcpy. Alternatively, in function, GetRelationPublicationInfo(), we can have an Assert when rd_rfcol_valid is true. I think we can add comments atop GetRelationPublicationInfo about pubactions. > > 13. src/include/utils/rel.h - comment typos > > @@ -164,6 +164,13 @@ typedef struct RelationData > PublicationActions *rd_pubactions; /* publication actions */ > > /* > + * true if the columns referenced in row filters from all the publications > + * the relation is in are part of replica identity, or the publication > + * actions do not include UPDATE and DELETE. > + */ > > Some minor rewording of the comment: > ... > "UPDATE and DELETE" --> "UPDATE or DELETE" > The existing comment seems correct to me. Hou-San can confirm it once as I think this is written by him. -- With Regards, Amit Kapila.
Re: daitch_mokotoff module
Thomas Munro writes: > On Wed, Jan 5, 2022 at 2:49 AM Dag Lem wrote: >> However I guess this won't make any difference wrt. actually running the >> tests, as long as there seems to be an encoding problem in the cfbot > > Fixed -- I told it to pull down patches as binary, not text. Now it > makes commits that look healthier, and so far all the Unix systems > have survived CI: > > https://github.com/postgresql-cfbot/postgresql/commit/79700efc61d15c2414b8450a786951fa9308c07f > http://cfbot.cputube.org/dag-lem.html > Great! Dag
RE: row filtering for logical replication
On Wednesday, January 5, 2022 2:45 PM Amit Kapila wrote: > > On Wed, Jan 5, 2022 at 11:04 AM Peter Smith > wrote: > > > > > > 11. src/backend/utils/cache/relcache.c - GetRelationPublicationActions > > > > Something seemed slightly fishy with the code doing the memcpy, > > because IIUC is possible for the GetRelationPublicationInfo function > > to return without setting the relation->rd_pubactions. Is it just > > missing an Assert or maybe a comment to say such a scenario is not > > possible in this case because the is_publishable_relation was already > > tested? > > > > I think it would be good to have an Assert for a valid value of > relation->rd_pubactions before doing memcpy. Alternatively, in > function, GetRelationPublicationInfo(), we can have an Assert when > rd_rfcol_valid is true. I think we can add comments atop > GetRelationPublicationInfo about pubactions. > > > > > 13. src/include/utils/rel.h - comment typos > > > > @@ -164,6 +164,13 @@ typedef struct RelationData > > PublicationActions *rd_pubactions; /* publication actions */ > > > > /* > > + * true if the columns referenced in row filters from all the > > + publications > > + * the relation is in are part of replica identity, or the > > + publication > > + * actions do not include UPDATE and DELETE. > > + */ > > > > Some minor rewording of the comment: > > > ... > > "UPDATE and DELETE" --> "UPDATE or DELETE" > > > > The existing comment seems correct to me. Hou-San can confirm it once as I > think this is written by him. I think the code comment is trying to say "the publication does not include UPDATE and also does not include DELETE" I am not too sure about the grammar, I noticed there is some other places in the code use " no updates or deletes ", so maybe it's fine to change it to "UPDATE or DELETE" Best regards, Hou zj
Re: Add jsonlog log_destination for JSON server logs
On Sun, Jan 02, 2022 at 01:34:45PM -0800, Andres Freund wrote:
> The tests don't seem to pass on windows:
> https://cirrus-ci.com/task/5412456754315264?logs=test_bin#L47
> https://api.cirrus-ci.com/v1/artifact/task/5412456754315264/tap/src/bin/pg_ctl/tmp_check/log/regress_log_004_logrotate
>
> psql::1: ERROR: division by zero
> could not open
> "c:/cirrus/src/bin/pg_ctl/tmp_check/t_004_logrotate_primary_data/pgdata/current_logfiles":
> The system cannot find the file specified at t/004_logrotate.pl line 87.
This seems to point out that the syslogger is too slow to capture the
logrotate signal, and the patch set is introducing nothing new in
terms of infrastructure, just an extra value for log_destination.
This stuff passes here, and I am not spotting something amiss after an
extra close read.
Attached is an updated patch set that increases the test timeout (5min
-> 10min). That should help, I assume.
--
Michael
From 33fbcbe4c8d4ac43a10d6f9362a961352a8df8ca Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 19 Oct 2021 16:25:45 +0900
Subject: [PATCH v8 1/3] Some refactoring of elog-specific routines
This refactors out the following things in elog.c, for ease of use
across multiple log destinations:
- start_timestamp (including reset)
- log_timestamp
- decide if query can be logged
- backend type
- write using the elog piped protocol
- Error severity to string.
These will be reused by csvlog and jsonlog.
---
src/include/utils/elog.h | 12 +++
src/backend/utils/error/elog.c | 159 +
2 files changed, 114 insertions(+), 57 deletions(-)
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index f53607e12e..731f3e3cd8 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -442,6 +442,18 @@ extern void DebugFileOpen(void);
extern char *unpack_sql_state(int sql_state);
extern bool in_error_recursion_trouble(void);
+/* Common functions shared across destinations */
+extern void reset_formatted_start_time(void);
+extern char *get_formatted_start_time(void);
+extern char *get_formatted_log_time(void);
+extern const char *get_backend_type_for_log(void);
+extern bool check_log_of_query(ErrorData *edata);
+extern const char *error_severity(int elevel);
+extern void write_pipe_chunks(char *data, int len, int dest);
+
+/* Destination-specific functions */
+extern void write_csvlog(ErrorData *edata);
+
#ifdef HAVE_SYSLOG
extern void set_syslog_parameters(const char *ident, int facility);
#endif
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index f33729513a..a162258bab 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -175,15 +175,10 @@ static const char *err_gettext(const char *str) pg_attribute_format_arg(1);
static pg_noinline void set_backtrace(ErrorData *edata, int num_skip);
static void set_errdata_field(MemoryContextData *cxt, char **ptr, const char *str);
static void write_console(const char *line, int len);
-static void setup_formatted_log_time(void);
-static void setup_formatted_start_time(void);
static const char *process_log_prefix_padding(const char *p, int *padding);
static void log_line_prefix(StringInfo buf, ErrorData *edata);
-static void write_csvlog(ErrorData *edata);
static void send_message_to_server_log(ErrorData *edata);
-static void write_pipe_chunks(char *data, int len, int dest);
static void send_message_to_frontend(ErrorData *edata);
-static const char *error_severity(int elevel);
static void append_with_tabs(StringInfo buf, const char *str);
@@ -2289,14 +2284,23 @@ write_console(const char *line, int len)
}
/*
- * setup formatted_log_time, for consistent times between CSV and regular logs
+ * get_formatted_log_time -- compute and get the log timestamp.
+ *
+ * The timestamp is computed if not set yet, so as it is kept consistent
+ * among all the log destinations that require it to be consistent. Note
+ * that the computed timestamp is returned in a static buffer, not
+ * palloc()'d.
*/
-static void
-setup_formatted_log_time(void)
+char *
+get_formatted_log_time(void)
{
pg_time_t stamp_time;
char msbuf[13];
+ /* leave if already computed */
+ if (formatted_log_time[0] != '\0')
+ return formatted_log_time;
+
if (!saved_timeval_set)
{
gettimeofday(&saved_timeval, NULL);
@@ -2318,16 +2322,34 @@ setup_formatted_log_time(void)
/* 'paste' milliseconds into place... */
sprintf(msbuf, ".%03d", (int) (saved_timeval.tv_usec / 1000));
memcpy(formatted_log_time + 19, msbuf, 4);
+
+ return formatted_log_time;
}
/*
- * setup formatted_start_time
+ * reset_formatted_start_time -- reset the start timestamp
*/
-static void
-setup_formatted_start_time(void)
+void
+reset_formatted_start_time(void)
+{
+ formatted_start_time[0] = '\0';
+}
+
+/*
+ * get_formatted_start_time -- compute and get the start timestamp.
+ *
+ * The timestamp is computed if not set yet. Note that the computed
+ * timestamp is returned in a
Re: support for MERGE
On Wed, 22 Dec 2021 at 11:35, Simon Riggs wrote: > > On Mon, 15 Nov 2021 at 22:45, Alvaro Herrera wrote: > > > > On 2021-Nov-15, Alvaro Herrera wrote: > > > > > Thanks everyone for the feedback. I attach a version with the fixes > > > that were submitted, as well as some additional changes: > > > > Attachment failure. > > I rebased this, please check. > > And 2 patch-on-patches that resolve a few minor questions/docs wording. Here is another patch-on-patch to fix a syntax error in the MERGE docs. -- Simon Riggshttp://www.EnterpriseDB.com/ merge_fix_docs_into.v1.patch Description: Binary data
