Re: Skipping logical replication transactions on subscriber side
On Mon, Nov 15, 2021 at 1:49 PM Masahiko Sawada wrote: > > I've attached an updated patch that incorporates all comments I got so > far. Please review it. > Thanks for the updated patch. A few minor comments: doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml (1) tab in doc updates There's a tab before "Otherwise,": +copy of the relation with relid. Otherwise, src/backend/utils/adt/pgstatfuncs.c (2) The function comment for "pg_stat_reset_subscription_worker_sub" seems a bit long and I expected it to be multi-line (did you run pg_indent?) src/include/pgstat.h (3) Remove PgStat_StatSubWorkerEntry.dbid? The "dbid" member of the new PgStat_StatSubWorkerEntry struct doesn't seem to be used, so I think it should be removed. (I could remove it and everything builds OK and tests pass). Regards, Greg Nancarrow Fujitsu Australia
Re: Printing backtrace of postgres processes
On Mon, Nov 15, 2021 at 12:08 PM Dilip Kumar wrote: > > > 2. > > > postgres[64154]=# select pg_print_backtrace(64136); > > > WARNING: 01000: PID 64136 is not a PostgreSQL server process > > > LOCATION: pg_print_backtrace, signalfuncs.c:335 > > > pg_print_backtrace > > > > > > f > > > > > > > > > For postmaster I am getting this WARNING "PID 64136 is not a > > > PostgreSQL server process", even if we don't want to support this > > > process I don't think this message is good. > > > > This is a generic message that is coming from pg_signal_backend, not > > related to Vignesh's patch. I agree with you that emitting a "not > > postgres server process" for the postmaster process which is the main > > "postgres process" doesn't sound sensible. Please see there's already > > a thread [1] and see the v1 patch [2] for changing this message. > > Please let me know if you want me to revive that stalled thread? > > >[1] > >https://www.postgresql.org/message-id/CALj2ACW7Rr-R7mBcBQiXWPp%3DJV5chajjTdudLiF5YcpW-BmHhg%40mail.gmail.com > >[2] > >https://www.postgresql.org/message-id/CALj2ACUGxedgYk-5nO8D2EJV2YHXnoycp_oqYAxDXTODhWkmkg%40mail.gmail.com > > Hmm, yeah I think I like the idea posted in [1], however, I could not > open the link [2] Thanks, I posted an updated v3 patch at [1]. Please review it there. [1] - https://www.postgresql.org/message-id/CALj2ACWS2bJRW-bSvcoL4FvS%3DkbQ8SSWXi%3D9RFUt7uqZvTQWWw%40mail.gmail.com Regards, Bharath Rupireddy.
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On Sun, Mar 7, 2021 at 3:46 PM Bharath Rupireddy wrote: > > On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy > wrote: > > > > pg_terminate_backend and pg_cancel_backend with postmaster PID produce > > "PID is not a PostgresSQL server process" warning [1], which > > basically implies that the postmaster is not a PostgreSQL process at > > all. This is a bit misleading because the postmaster is the parent of > > all PostgreSQL processes. Should we improve the warning message if the > > given PID is postmasters' PID? > > > > If yes, how about a generic message for both of the functions - > > "signalling postmaster process is not allowed" or "cannot signal > > postmaster process" or some other better suggestion? > > > > [1] 2471176 ---> is postmaster PID. > > postgres=# select pg_terminate_backend(2471176); > > WARNING: PID 2471176 is not a PostgreSQL server process > > pg_terminate_backend > > -- > > f > > (1 row) > > postgres=# select pg_cancel_backend(2471176); > > WARNING: PID 2471176 is not a PostgreSQL server process > > pg_cancel_backend > > --- > > f > > (1 row) > > I'm attaching a small patch that emits a warning "signalling > postmaster with PID %d is not allowed" for postmaster and "signalling > PostgreSQL server process with PID %d is not allowed" for auxiliary > processes such as checkpointer, background writer, walwriter. > > However, for stats collector and sys logger processes, we still get > "PID X is not a PostgreSQL server process" warning because they > don't have PGPROC entries(??). So BackendPidGetProc and > AuxiliaryPidGetProc will not help and even pg_stat_activity is not > having these processes' pid. As there is some interest shown in this thread at [1], I'm attaching a new v3 patch here. Please review it. CF entry - https://commitfest.postgresql.org/36/3411/ [1] - https://www.postgresql.org/message-id/CAFiTN-sX_66svOPdix1edB_WxGj%3DWu4XouyRQrvySwCK0V8Btg%40mail.gmail.com Regards, Bharath Rupireddy. v3-0001-Improve-PID--is-not-a-PostgreSQL-server-proce.patch Description: Binary data
Re: Parallel vacuum workers prevent the oldest xmin from advancing
On Sat, Nov 13, 2021 at 2:10 PM Amit Kapila wrote: > > On Fri, Nov 12, 2021 at 6:44 PM Alvaro Herrera > wrote: > > > > On 2021-Nov-11, Masahiko Sawada wrote: > > > > > On Thu, Nov 11, 2021 at 12:53 PM Amit Kapila > > > wrote: > > > > > > > > On Thu, Nov 11, 2021 at 9:11 AM Andres Freund > > > > wrote: > > > > > > > This seems like an unnecessary optimization. > > > > > ProcArrayInstallRestoredXmin() > > > > > only happens in the context of much more expensive operations. > > > > > > > > Fair point. I think that will also make the change in > > > > ProcArrayInstallRestoredXmin() appear neat. > > > > > > This makes me think that it'd be better to copy status flags in a > > > separate function rather than ProcArrayInstallRestoredXmin(). > > Thank you for the comment! > > To me, and this is perhaps just personal opinion, it seems conceptually > > simpler to have ProcArrayInstallRestoredXmin acquire exclusive and do > > both things. Why? Because if you have two functions, you have to be > > careful not to call the new function after ProcArrayInstallRestoredXmin; > > otherwise you would create an instant during which you make an > > Xmin-without-flag visible to other procs; this causes the computed xmin > > go backwards, which is verboten. I agree that it's simpler. I thought statusFlags and xmin are conceptually separate things since PROC_VACUUM_FOR_WRAPAROUND is not related to xid at all for example. But given that the use case of copying statusFlags from someone is only parallel worker startup for now, copying statusFlags while setting xmin seems convenient and simple. If we want to only copy statusFlags in some use cases in the future, we can have a separate function for that. > > > > If I understand Amit correctly, his point is about the callers of > > RestoreTransactionSnapshot, which are two: CreateReplicationSlot and > > ParallelWorkerMain. He wants you hypothetical new function called from > > the latter but not the former. Looking at both, it seems a bit strange > > to make them responsible for a detail such as "copy ->statusFlags from > > source proc to mine". It seems more reasonable to add a third flag to > > RestoreTransactionSnapshot(Snapshot snapshot, void *source_proc, bool > > is_vacuum) > > and if that is true, tell SetTransactionSnapshot to copy flags, > > otherwise not. For the idea of is_vacuum flag, we don't know if a parallel worker is launched for parallel vacuum at the time of ParallelWorkerMain(). > > > > If we decide to go this way then I suggest adding a comment to convey > why we choose to copy status flags in ProcArrayInstallRestoredXmin() > as the function name doesn't indicate it. Agreed. I've updated the patch so that ProcArrayInstallRestoredXmin() sets both xmin and statusFlags only when the source proc is still running and xmin doesn't go backwards. IOW it doesn't happen that only one of them is set by this function, which seems more understandable behavior. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ copy_status_flags_v4.patch Description: Binary data
Re: row filtering for logical replication
On Fri, Nov 12, 2021 at 3:49 PM Ajin Cherian wrote: > > Attaching version 39- > > V39 fixes the following review comments: > > On Fri, Nov 5, 2021 at 7:49 PM Amit Kapila wrote: > > > > CheckObjSchemaNotAlreadyInPublication(rels, schemaidlist, > >PUBLICATIONOBJ_TABLE); > > > >I think for the correct merge you need to just call > >CheckObjSchemaNotAlreadyInPublication() before this for loop. BTW, I > >have a question regarding this implementation. Here, it has been > >assumed that the new rel will always be specified with a different > >qual, what if there is no qual or if the qual is the same? > > Actually with this code, no qual or a different qual does not matter, > it recreates everything as specified by the ALTER SET command. > I have added CheckObjSchemaNotAlreadyInPublication as you specified since this > is required to match the schema patch behaviour. I've also added > a test case that tests this particular case. > What I meant was that with this new code we have regressed the old behavior. Basically, imagine a case where no filter was given for any of the tables. Then after the patch, we will remove all the old tables whereas before the patch it will remove the oldrels only when they are not specified as part of new rels. If you agree with this, then we can retain the old behavior and for the new tables, we can always override the where clause for a SET variant of command. -- With Regards, Amit Kapila.
Re: Printing backtrace of postgres processes
On Mon, Nov 15, 2021 at 12:13 PM vignesh C wrote: > > On Mon, Nov 15, 2021 at 11:00 AM Bharath Rupireddy > wrote: > > > > On Mon, Nov 15, 2021 at 10:34 AM vignesh C wrote: > > > > 2) I think "which is enough because the target process for logging of > > > > backtrace is a backend" isn't valid anymore with 0002, righit? Please > > > > remove it. > > > > + * to call this function if we see PrintBacktracePending set. It is > > > > called from > > > > + * CHECK_FOR_INTERRUPTS() or from process specific interrupt handlers, > > > > which is > > > > + * enough because the target process for logging of backtrace is a > > > > backend. > > > > > > > > > Thanks for the comments, v11 patch attached at [1] has the changes > > > > > for the same. > > > > > > Modified > > > > I don't see the above change in v12. Am I missing something? I still > > see the comment "It is called from CHECK_FOR_INTERRUPTS(), which is > > enough because the target process for logging of backtrace is a > > backend.". > > This change is present in the 0002 > (v12-0002-pg_print_backtrace-support-for-printing-backtrac.patch) > patch. Thanks. Yes, it was removed in 0002. I missed it. Regards, Bharath Rupireddy.
Re: Printing backtrace of postgres processes
On Mon, Nov 15, 2021 at 11:00 AM Bharath Rupireddy wrote: > > On Mon, Nov 15, 2021 at 10:34 AM vignesh C wrote: > > > 2) I think "which is enough because the target process for logging of > > > backtrace is a backend" isn't valid anymore with 0002, righit? Please > > > remove it. > > > + * to call this function if we see PrintBacktracePending set. It is > > > called from > > > + * CHECK_FOR_INTERRUPTS() or from process specific interrupt handlers, > > > which is > > > + * enough because the target process for logging of backtrace is a > > > backend. > > > > > > > Thanks for the comments, v11 patch attached at [1] has the changes for > > > > the same. > > > > Modified > > I don't see the above change in v12. Am I missing something? I still > see the comment "It is called from CHECK_FOR_INTERRUPTS(), which is > enough because the target process for logging of backtrace is a > backend.". This change is present in the 0002 (v12-0002-pg_print_backtrace-support-for-printing-backtrac.patch) patch. Regards, Vignesh
Re: Printing backtrace of postgres processes
On Mon, Nov 15, 2021 at 11:53 AM Bharath Rupireddy wrote: > > On Mon, Nov 15, 2021 at 11:37 AM Dilip Kumar wrote: > > > > On Mon, Nov 15, 2021 at 10:34 AM vignesh C wrote: > > > > > > > > Thanks for the comments, the attached v12 patch has the changes for the > > > same. > > > > I have reviewed this patch and have some comments on v12-0001, > > > > 1. > > +This feature is not supported for the postmaster, logger, > > checkpointer, > > +walwriter, background writer or statistics collector process. This > > > > > > Comment says it is not supported for postmaster, logger, checkpointer > > etc, but I just tried and it is working for checkpointer and walwriter > > processes, can you explain in comments why do we not want to support > > for these processes? or the comment is old and now we are supporting > > for some of these processes. > > Please see the v12-0002 which will have the description modified. Okay, now I see that. > > 2. > > postgres[64154]=# select pg_print_backtrace(64136); > > WARNING: 01000: PID 64136 is not a PostgreSQL server process > > LOCATION: pg_print_backtrace, signalfuncs.c:335 > > pg_print_backtrace > > > > f > > > > > > For postmaster I am getting this WARNING "PID 64136 is not a > > PostgreSQL server process", even if we don't want to support this > > process I don't think this message is good. > > This is a generic message that is coming from pg_signal_backend, not > related to Vignesh's patch. I agree with you that emitting a "not > postgres server process" for the postmaster process which is the main > "postgres process" doesn't sound sensible. Please see there's already > a thread [1] and see the v1 patch [2] for changing this message. > Please let me know if you want me to revive that stalled thread? >[1] >https://www.postgresql.org/message-id/CALj2ACW7Rr-R7mBcBQiXWPp%3DJV5chajjTdudLiF5YcpW-BmHhg%40mail.gmail.com >[2] >https://www.postgresql.org/message-id/CALj2ACUGxedgYk-5nO8D2EJV2YHXnoycp_oqYAxDXTODhWkmkg%40mail.gmail.com Hmm, yeah I think I like the idea posted in [1], however, I could not open the link [2] -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Printing backtrace of postgres processes
On Mon, Nov 15, 2021 at 11:37 AM Dilip Kumar wrote: > > On Mon, Nov 15, 2021 at 10:34 AM vignesh C wrote: > > > > > Thanks for the comments, the attached v12 patch has the changes for the > > same. > > I have reviewed this patch and have some comments on v12-0001, > > 1. > +This feature is not supported for the postmaster, logger, > checkpointer, > +walwriter, background writer or statistics collector process. This > > > Comment says it is not supported for postmaster, logger, checkpointer > etc, but I just tried and it is working for checkpointer and walwriter > processes, can you explain in comments why do we not want to support > for these processes? or the comment is old and now we are supporting > for some of these processes. Please see the v12-0002 which will have the description modified. > 2. > postgres[64154]=# select pg_print_backtrace(64136); > WARNING: 01000: PID 64136 is not a PostgreSQL server process > LOCATION: pg_print_backtrace, signalfuncs.c:335 > pg_print_backtrace > > f > > > For postmaster I am getting this WARNING "PID 64136 is not a > PostgreSQL server process", even if we don't want to support this > process I don't think this message is good. This is a generic message that is coming from pg_signal_backend, not related to Vignesh's patch. I agree with you that emitting a "not postgres server process" for the postmaster process which is the main "postgres process" doesn't sound sensible. Please see there's already a thread [1] and see the v1 patch [2] for changing this message. Please let me know if you want me to revive that stalled thread? [1] https://www.postgresql.org/message-id/CALj2ACW7Rr-R7mBcBQiXWPp%3DJV5chajjTdudLiF5YcpW-BmHhg%40mail.gmail.com [2] https://www.postgresql.org/message-id/CALj2ACUGxedgYk-5nO8D2EJV2YHXnoycp_oqYAxDXTODhWkmkg%40mail.gmail.com Regards, Bharath Rupireddy.
Re: Emit a warning if the extension's GUC is set incorrectly
On Mon, Nov 15, 2021 at 6:33 AM Shinya Kato
wrote:
>
> On 2021-11-15 04:50, Daniel Gustafsson wrote:
> > Seems reasonable on a quick skim, commit da2c1b8a2 did a similar
> > roundup back
> > in 2009 but at the time most of these didn't exist (pg_trgm did but
> > didn't have
> > custom option back then). There is one additional callsite defining
> > custom
> > variables in src/pl/tcl/pltcl.c which probably should get this
> > treatment as
> > well, it would align it with the pl/perl counterpart.
> >
> > I'll have a closer look and test tomorrow.
> Thank you for the review!
> I have missed src/pl/tcl/pltcl.c, so I created the new patch.
Do we need to add them in the following too?
delay_execution/delay_execution.c
ssl_passphrase_func.c
worker_spi.c
Especially, worker_spi.c is important as it works as a template for
the bg worker code.
I'm not sure if we have "how to create an extension/a bg worker?" in
the core docs, if we have, it is a good idea to make note of using
EmitWarningsOnPlaceholders so that it will not be missed in the future
modules.
I observed an odd behaviour:
1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
contrib module, I created the extension, have seen the following
warning:
2021-11-15 06:02:31.198 UTC [2018111] WARNING: unrecognized
configuration parameter "postgres_fdw.XXX"
3) I further did, "alter system set
postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
pg_reload_conf();", it silently accepts.
postgres=# create extension postgres_fdw ;
WARNING: unrecognized configuration parameter "postgres_fdw.XXX"
CREATE EXTENSION
postgres=# alter system set postgres_fdw.XXX='I_further_messed_up_conf_file';
ALTER SYSTEM
postgres=# select pg_reload_conf();
pg_reload_conf
t
(1 row)
My point is, why should the "create extension" succeed with a WARNING
when a wrong parameter related to it is set in the postgresql.conf
file so that we don't see further problems as shown in (3). I think
EmitWarningsOnPlaceholders should emit an error so that the extension
will not get created in the first place if a wrong configuration
parameter is set in the postgresql.conf file. Many times, users will
not have access to server logs so it is good to fail the "create
extension" statement.
Thoughts?
postgres=# create extension postgres_fdw ;
ERROR: unrecognized configuration parameter "postgres_fdw.XXX"
postgres=#
Regards,
Bharath Rupireddy.
RE: row filtering for logical replication
On Friday, November 12, 2021 6:20 PM Ajin Cherian wrote: > > Attaching version 39- > Thanks for the new patch. I met a problem when using "ALTER PUBLICATION ... SET TABLE ... WHERE ...", the publisher was crashed after executing this statement. Here is some information about this problem. Steps to reproduce: -- publisher create table t(a int primary key, b int); create publication pub for table t where (a>5); -- subscriber create table t(a int primary key, b int); create subscription sub connection 'dbname=postgres port=5432' publication pub; -- publisher insert into t values (1, 2); alter publication pub set table t where (a>7); Publisher log: 2021-11-15 13:36:54.997 CST [3319891] LOG: logical decoding found consistent point at 0/15208B8 2021-11-15 13:36:54.997 CST [3319891] DETAIL: There are no running transactions. 2021-11-15 13:36:54.997 CST [3319891] STATEMENT: START_REPLICATION SLOT "sub" LOGICAL 0/0 (proto_version '3', publication_names '"pub"') double free or corruption (out) 2021-11-15 13:36:55.072 CST [3319746] LOG: received fast shutdown request 2021-11-15 13:36:55.073 CST [3319746] LOG: aborting any active transactions 2021-11-15 13:36:55.105 CST [3319746] LOG: background worker "logical replication launcher" (PID 3319874) exited with exit code 1 2021-11-15 13:36:55.105 CST [3319869] LOG: shutting down 2021-11-15 13:36:55.554 CST [3319746] LOG: server process (PID 3319891) was terminated by signal 6: Aborted 2021-11-15 13:36:55.554 CST [3319746] DETAIL: Failed process was running: START_REPLICATION SLOT "sub" LOGICAL 0/0 (proto_version '3', publication_names '"pub"') 2021-11-15 13:36:55.554 CST [3319746] LOG: terminating any other active server processes Backtrace is attached. I think maybe the problem is related to the below change in 0003 patch: + free(entry->exprstate); Regards Tang (gdb) bt #0 0x7f132335970f in raise () from /lib64/libc.so.6 #1 0x7f1323343b25 in abort () from /lib64/libc.so.6 #2 0x7f132339c897 in __libc_message () from /lib64/libc.so.6 #3 0x7f13233a2fdc in malloc_printerr () from /lib64/libc.so.6 #4 0x7f13233a4d80 in _int_free () from /lib64/libc.so.6 #5 0x7f1317d4de6a in rel_sync_cache_relation_cb (arg=, relid=) at pgoutput.c:1884 #6 0x00dd4191 in LocalExecuteInvalidationMessage (msg=msg@entry=0x2f6c7c8) at inval.c:653 #7 0x00accfc2 in ReorderBufferExecuteInvalidations (nmsgs=4, msgs=0x2f6c7a8) at reorderbuffer.c:3261 #8 0x00ad1f6f in ReorderBufferProcessTXN (rb=rb@entry=0x2f6c4a0, txn=0x2f8a478, commit_lsn=commit_lsn@entry=22148728, snapshot_now=, command_id=command_id@entry=0, streaming=streaming@entry=false) at reorderbuffer.c:2333 #9 0x00ad35f6 in ReorderBufferReplay (txn=, rb=rb@entry=0x2f6c4a0, xid=xid@entry=713, commit_lsn=commit_lsn@entry=22148728, end_lsn=end_lsn@entry=22148912, commit_time=commit_time@entry=690256439713242, origin_id=0, origin_lsn=0) at reorderbuffer.c:2622 #10 0x00ad4200 in ReorderBufferCommit (rb=0x2f6c4a0, xid=xid@entry=713, commit_lsn=22148728, end_lsn=22148912, commit_time=commit_time@entry=690256439713242, origin_id=origin_id@entry=0, origin_lsn=0) at reorderbuffer.c:2646 #11 0x00ab1b59 in DecodeCommit (ctx=ctx@entry=0x2f6a490, buf=buf@entry=0x7ffd7dc0a6c0, parsed=parsed@entry=0x7ffd7dc0a550, xid=xid@entry=713, two_phase=) at decode.c:744 #12 0x00ab20c3 in DecodeXactOp (ctx=ctx@entry=0x2f6a490, buf=buf@entry=0x7ffd7dc0a6c0) at decode.c:279 #13 0x00ab3fd9 in LogicalDecodingProcessRecord (ctx=0x2f6a490, record=0x2f6a850) at decode.c:142 #14 0x00b0d23d in XLogSendLogical () at walsender.c:2992 #15 0x00b11ba3 in WalSndLoop (send_data=send_data@entry=0xb0d1d5 ) at walsender.c:2422 #16 0x00b122a5 in StartLogicalReplication (cmd=cmd@entry=0x2f26e38) at walsender.c:1315 #17 0x00b13b18 in exec_replication_command ( cmd_string=cmd_string@entry=0x2e8f590 "START_REPLICATION SLOT \"sub\" LOGICAL 0/0 (proto_version '3', publication_names '\"pub\"')") at walsender.c:1762 #18 0x00bbae43 in PostgresMain (dbname=, username=) at postgres.c:4493 #19 0x00a84906 in BackendRun (port=port@entry=0x2eb5580) at postmaster.c:4584 #20 0x00a8ae73 in BackendStartup (port=port@entry=0x2eb5580) at postmaster.c:4312 #21 0x00a8b2b2 in ServerLoop () at postmaster.c:1801 #22 0x00a8e010 in PostmasterMain (argc=argc@entry=3, argv=argv@entry=0x2e88b00) at postmaster.c:1473 #23 0x0091cb1a in main (argc=3, argv=0x2e88b00) at main.c:198
Re: Printing backtrace of postgres processes
On Mon, Nov 15, 2021 at 10:34 AM vignesh C wrote: > > Thanks for the comments, the attached v12 patch has the changes for the same. I have reviewed this patch and have some comments on v12-0001, 1. +This feature is not supported for the postmaster, logger, checkpointer, +walwriter, background writer or statistics collector process. This Comment says it is not supported for postmaster, logger, checkpointer etc, but I just tried and it is working for checkpointer and walwriter processes, can you explain in comments why do we not want to support for these processes? or the comment is old and now we are supporting for some of these processes. 2. postgres[64154]=# select pg_print_backtrace(64136); WARNING: 01000: PID 64136 is not a PostgreSQL server process LOCATION: pg_print_backtrace, signalfuncs.c:335 pg_print_backtrace f For postmaster I am getting this WARNING "PID 64136 is not a PostgreSQL server process", even if we don't want to support this process I don't think this message is good. 3. I was looking into the patch, I tried to print the backtrace using GDB as well as using this function, I have complied in debug mode, I can see the backtrace printed by GDB is more detailed than printed by this API, I understand we can find out the function name from address, but can't we print the detailed call stack with all function names at least when debug symbols are available? Using GDB #0 0x7fa26c527e93 in __epoll_wait_nocancel () from #1 0x00947a61 in WaitEventSetWaitBlock (set=0x2 #2 0x009478f9 in WaitEventSetWait (set=0x2f0111 #3 0x007a6cef in secure_read (port=0x2f26800, p #4 0x007b0bd6 in pq_recvbuf () at pqcomm.c:957 #5 0x007b0c86 in pq_getbyte () at pqcomm.c:1000 #6 0x00978c13 in SocketBackend (inBuf=0x7ffea99 #7 0x00978e37 in ReadCommand (inBuf=0x7ffea9937 #8 0x0097dca5 in PostgresMain (dbname=0x2f2ef40 Using pg_print_backtrace postgres: dilipkumar postgres [local] SELECT(set_backtrace+0x38) [0xb118ff] postgres: dilipkumar postgres [local] SELECT(ProcessPrintBacktraceInterrupt+0x5b) [0x94fe42] postgres: dilipkumar postgres [local] SELECT(ProcessInterrupts+0x7d9) [0x97cb2a] postgres: dilipkumar postgres [local] SELECT() [0x78143c] postgres: dilipkumar postgres [local] SELECT() [0x736731] postgres: dilipkumar postgres [local] SELECT() [0x738f5f] postgres: dilipkumar postgres [local] SELECT(standard_ExecutorRun+0x1d6) [0x736d94] postgres: dilipkumar postgres [local] SELECT(ExecutorRun+0x55) [0x736bbc] postgres: dilipkumar postgres [local] SELECT() [0x97ff0c] postgres: dilipkumar postgres [local] SELECT(PortalRun+0x268) [0x97fbbf] postgres: dilipkumar postgres [local] SELECT() [0x9798dc] postgres: dilipkumar postgres [local] SELECT(PostgresMain+0x6ca) [0x97dda7] 4. +WARNING: backtrace generation is not supported by this installation + pg_print_backtrace + + f Should we give some hints on how to enable that? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
RE: row filtering for logical replication
On Wednesday, November 10, 2021 7:46 AM Peter Smith wrote: > > On Tue, Nov 9, 2021 at 2:03 PM tanghy.f...@fujitsu.com > wrote: > > > > On Friday, November 5, 2021 1:14 PM, Peter Smith > wrote: > > > > > > PSA new set of v37* patches. > > > > > > > Thanks for your patch. I have a problem when using this patch. > > > > The document about "create publication" in patch says: > > > >The WHERE clause should contain only columns that are > >part of the primary key or be covered by REPLICA > >IDENTITY otherwise, DELETE operations will > not > >be replicated. > > > > But I tried this patch, the columns which could be contained in WHERE clause > must be > > covered by REPLICA IDENTITY, but it doesn't matter if they are part of the > primary key. > > (We can see it in Case 4 of publication.sql, too.) So maybe we should > > modify the > document. > > > > PG Docs is changed in v38-0004 [1]. Please check if it is OK. > Thanks, this change looks good to me. Regards Tang
Re: Reuse of State value in Aggregates
That's sufficient information for me thanks! I don't really have a concrete example at the moment. The idea just popped up in my head when I read that CTEs are reused inside parent queries.
Re: Printing backtrace of postgres processes
On Mon, Nov 15, 2021 at 10:34 AM vignesh C wrote: > > 2) I think "which is enough because the target process for logging of > > backtrace is a backend" isn't valid anymore with 0002, righit? Please > > remove it. > > + * to call this function if we see PrintBacktracePending set. It is called > > from > > + * CHECK_FOR_INTERRUPTS() or from process specific interrupt handlers, > > which is > > + * enough because the target process for logging of backtrace is a backend. > > > > > Thanks for the comments, v11 patch attached at [1] has the changes for > > > the same. > > Modified I don't see the above change in v12. Am I missing something? I still see the comment "It is called from CHECK_FOR_INTERRUPTS(), which is enough because the target process for logging of backtrace is a backend.". + * ProcessPrintBacktraceInterrupt - Perform logging of backtrace of this + * backend process. + * + * Any backend that participates in ProcSignal signaling must arrange + * to call this function if we see PrintBacktracePending set. + * It is called from CHECK_FOR_INTERRUPTS(), which is enough because + * the target process for logging of backtrace is a backend. + */ +void +ProcessPrintBacktraceInterrupt(void) Regards, Bharath Rupireddy.
Re: Printing backtrace of postgres processes
On Mon, Nov 15, 2021 at 7:37 AM Bharath Rupireddy
wrote:
>
> On Sun, Nov 14, 2021 at 8:49 PM vignesh C wrote:
> > > 7) Do we need TAP tests for this function? I think it is sufficient to
> > > test the function in misc_functions.sql, please remove
> > > 002_print_backtrace_validation.pl. Note that we don't do similar TAP
> > > testing for pg_log_backend_memory_contexts as well.
> >
> > I felt let's keep this test case, all the other tests just check if it
> > returns true or false, it does not checks for the contents in the
> > logfile. This is the only test which checks the logfile.
>
> I still don't agree to have test cases within a new file
> 002_print_backtrace_validation.pl. I feel this test case doesn't add
> value because the code coverage is done by .sql test cases and .pl
> just ensures the backtrace appears in the server logs. I don't think
> we ever need a new file for this purpose. If this is the case, then
> there are other functions like pg_log_backend_memory_contexts or
> pg_log_query_plan (in progress thread) might add the same test files
> for the same reasons which make the TAP tests i.e. "make check-world"
> to take longer times. Moreover, pg_log_backend_memory_contexts has
> been committed without having a TAP test case.
>
> I think we can remove it.
Removed
> Few more comments on v11:
> 1) I think we can improve here by adding a link to "backend" as well,
> I will modify it in the other thread.
> +Requests to log the backtrace of the backend or the
> +WAL sender or
> Something like:
> +Requests to log the backtrace of the linkend="glossary-backend">backend or the
> +WAL sender or
Modified
> 2) I think "which is enough because the target process for logging of
> backtrace is a backend" isn't valid anymore with 0002, righit? Please
> remove it.
> + * to call this function if we see PrintBacktracePending set. It is called
> from
> + * CHECK_FOR_INTERRUPTS() or from process specific interrupt handlers, which
> is
> + * enough because the target process for logging of backtrace is a backend.
>
> > Thanks for the comments, v11 patch attached at [1] has the changes for the
> > same.
Modified
Thanks for the comments, the attached v12 patch has the changes for the same.
Regards,
Vignesh
From c34d0114833219ef7b1b2d4283522ce9d4e6ffe3 Mon Sep 17 00:00:00 2001
From: Vigneshwaran C
Date: Tue, 9 Nov 2021 16:28:03 +0530
Subject: [PATCH v12 1/2] Print backtrace of specified postgres process.
The idea here is to implement & expose pg_print_backtrace function, internally
what this function does is, the connected backend will send SIGUSR1 signal by
setting PROCSIG_PRINT_BACKTRACE to postgres backend whose pid matches the
specified process id. Once the backend process receives this signal it will
print the backtrace of the process to the log file based on the logging
configuration, if logging is disabled backtrace will be printed to the
console where postmaster was started.
---
doc/src/sgml/func.sgml | 83
src/backend/catalog/system_functions.sql | 2 +
src/backend/postmaster/autovacuum.c | 4 +
src/backend/storage/ipc/procsignal.c | 41 ++
src/backend/storage/ipc/signalfuncs.c| 48 +++
src/backend/tcop/postgres.c | 4 +
src/backend/utils/error/elog.c | 20 -
src/backend/utils/init/globals.c | 1 +
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.dat | 5 ++
src/include/miscadmin.h | 1 +
src/include/storage/procsignal.h | 3 +-
src/include/utils/elog.h | 2 +
src/test/regress/expected/misc_functions.out | 42 ++
src/test/regress/sql/misc_functions.sql | 31
15 files changed, 283 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 24447c0017..8d147825eb 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25345,6 +25345,32 @@ SELECT collation for ('foo' COLLATE "de_DE");
+
+
+
+ pg_print_backtrace
+
+pg_print_backtrace ( pid integer )
+boolean
+
+
+Requests to log the backtrace of the
+backend
+with the specified process ID. The backtrace will be logged at message
+level LOG. It will appear in the server log based on
+the log configuration set (See
+for more information), but will not be sent to the client regardless of
+. A backtrace will identify
+where exactly the backend process is currently executing. This may be
+useful to developers to diagnose stuck processes and other problems.
+This feature is not supported for the postmaster, logger, checkpointer,
+walwriter, background writer or statistics collector process. This
+feature will be available if
Re: Add psql command to list constraints
Hi,
On Mon, Nov 15, 2021 at 10:38:55AM +0900, Tatsuro Yamada wrote:
> postgres=# \dco
> List of constsraints
> Schema | Name | Definition
> | Table
> +-+-+--
> public | t01_chk_price_check | CHECK ((price > (0)::numeric))
> | t01_chk
> public | t02_uniq_product_no_key | UNIQUE (product_no)
> | t02_uniq
> public | t03_pk1_pkey| PRIMARY KEY (product_no)
> | t03_pk1
> public | t03_pk2_product_no_key | UNIQUE (product_no)
> | t03_pk2
> public | t04_fk_pkey | PRIMARY KEY (order_id)
> | t04_fk
> public | t04_fk_product_no_fkey | FOREIGN KEY (product_no) REFERENCES
> t03_pk1(product_no) | t04_fk
> public | t05_ex_c_excl | EXCLUDE USING gist (c WITH &&)
> | t05_ex
> (7 rows)
>
Maybe it ought to be possible to choose the type of constraints to show.
Similar to how \dt shows tables and \di shows indexes and \dti shows
tables+inds, you could run \dcoc for check constraints and \dcof for foreign
keys. But I think "\dco" is too long of a prefix...
> + initPQExpBuffer();
> + printfPQExpBuffer(,
> + "SELECT \n"
> + "n.nspname AS \"%s\", \n"
> + "cst.conname AS \"%s\", \n"
> +
> "pg_catalog.pg_get_constraintdef(cst.oid) AS \"%s\", \n"
> + "c.relname AS \"%s\" \n"
> + "FROM pg_constraint cst \n"
> + "JOIN pg_namespace n ON n.oid =
> cst.connamespace \n"
> + "JOIN pg_class c ON c.oid =
> cst.conrelid \n",
You should write "pg_catalog." prefix for the tables (in addition to the
function).
Rather than join to pg_class, you can write conrelid::pg_catalog.regclass,
since regclass is supported since at least v7.3 (but ::regnamespace was
introduced in v9.5, so the join against pg_namespace is still necessary).
https://www.postgresql.org/docs/9.5/datatype-oid.html
> + myopt.title = _("List of constsraints");
spelling: constraints
I'm not confident that if I would use this, so let's wait to see if someone
else wants to give a +1.
--
Justin
Re: Clean up build warnings of plperl with clang-12+
On Thu, Nov 11, 2021 at 03:51:35PM -0500, Tom Lane wrote: > That'd be considerably messier wouldn't it? Yes, that would be a bit messier. For example, we could do that with something saved in Makefile.global.in by ./configure that plperl feeds on to add this extra CFLAGS in its own local Makefile. > (And I wonder if the > meson system will be able to do it at all.) If you feel motivated > to make that happen, go for it, but I don't. Neither am I. 9ff47ea is the least invasive solution, at least. -- Michael signature.asc Description: PGP signature
Re: Skipping logical replication transactions on subscriber side
On Wed, Nov 10, 2021 at 12:49 PM vignesh C wrote:
>
>
> Thanks for the updated patch, Few comments:
Thank you for the comments!
> 1) should we change "Tables and functions hashes are initialized to
> empty" to "Tables, functions and subworker hashes are initialized to
> empty"
> + hash_ctl.keysize = sizeof(PgStat_StatSubWorkerKey);
> + hash_ctl.entrysize = sizeof(PgStat_StatSubWorkerEntry);
> + dbentry->subworkers = hash_create("Per-database subscription worker",
> +
>PGSTAT_SUBWORKER_HASH_SIZE,
> +
>_ctl,
> +
>HASH_ELEM | HASH_BLOBS);
Fixed.
>
> 2) Since databaseid, tabhash, funchash and subworkerhash are members
> of dbentry, can we remove the function arguments databaseid, tabhash,
> funchash and subworkerhash and pass dbentry similar to
> pgstat_write_db_statsfile function?
> @@ -4370,12 +4582,14 @@ done:
> */
> static void
> pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash,
> -bool permanent)
> +HTAB *subworkerhash,
> bool permanent)
> {
> PgStat_StatTabEntry *tabentry;
> PgStat_StatTabEntry tabbuf;
> PgStat_StatFuncEntry funcbuf;
> PgStat_StatFuncEntry *funcentry;
> + PgStat_StatSubWorkerEntry subwbuf;
> + PgStat_StatSubWorkerEntry *subwentry;
>
As the comment of this function says, this function has the ability to
skip storing per-table or per-function (and or
per-subscription-workers) data, if NULL is passed for the
corresponding hashtable, although that's not used at the moment. IMO
it'd be better to keep such behavior.
> 3) Can we move pgstat_get_subworker_entry below pgstat_get_db_entry
> and pgstat_get_tab_entry, so that the hash lookup can be together
> consistently. Similarly pgstat_send_subscription_purge can be moved
> after pgstat_send_slru.
> +/* --
> + * pgstat_get_subworker_entry
> + *
> + * Return subscription worker entry with the given subscription OID and
> + * relation OID. If subrelid is InvalidOid, it returns an entry of the
> + * apply worker otherwise of the table sync worker associated with subrelid.
> + * If no subscription entry exists, initialize it, if the create parameter
> + * is true. Else, return NULL.
> + * --
> + */
> +static PgStat_StatSubWorkerEntry *
> +pgstat_get_subworker_entry(PgStat_StatDBEntry *dbentry, Oid subid,
> Oid subrelid,
> + bool create)
> +{
> + PgStat_StatSubWorkerEntry *subwentry;
> + PgStat_StatSubWorkerKey key;
> + boolfound;
Agreed. Moved.
>
> 4) This change can be removed from pgstat.c:
> @@ -332,9 +339,11 @@ static bool pgstat_db_requested(Oid databaseid);
> static PgStat_StatReplSlotEntry *pgstat_get_replslot_entry(NameData
> name, bool create_it);
> static void pgstat_reset_replslot(PgStat_StatReplSlotEntry
> *slotstats, TimestampTz ts);
>
> +
> static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg, TimestampTz now);
> static void pgstat_send_funcstats(void);
Removed.
>
> 5) I was able to compile without including
> catalog/pg_subscription_rel.h, we can remove including
> catalog/pg_subscription_rel.h if not required.
> --- a/src/backend/postmaster/pgstat.c
> +++ b/src/backend/postmaster/pgstat.c
> @@ -41,6 +41,8 @@
> #include "catalog/catalog.h"
> #include "catalog/pg_database.h"
> #include "catalog/pg_proc.h"
> +#include "catalog/pg_subscription.h"
> +#include "catalog/pg_subscription_rel.h"
Removed.
>
> 6) Similarly replication/logicalproto.h also need not be included
> --- a/src/backend/utils/adt/pgstatfuncs.c
> +++ b/src/backend/utils/adt/pgstatfuncs.c
> @@ -24,6 +24,7 @@
> #include "pgstat.h"
> #include "postmaster/bgworker_internals.h"
> #include "postmaster/postmaster.h"
> +#include "replication/logicalproto.h"
> #include "replication/slot.h"
> #include "storage/proc.h"
Removed;
>
> 7) There is an extra ";", We can remove one ";" from below:
> + PgStat_StatSubWorkerKey key;
> + boolfound;
> + HASHACTION action = (create ? HASH_ENTER : HASH_FIND);;
> +
> + key.subid = subid;
> + key.subrelid = subrelid;
Fixed.
I've attached an updated patch that incorporates all comments I got so
far. Please review it.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
v21-0001-Add-a-subscription-worker-statistics-view-pg_sta.patch
Description: Binary data
Re: Time to drop plpython2?
... btw, there's a fairly critical gating factor for any plan to drop python2 support: the buildfarm. I just counted, and there are exactly as many members running python 2.x as 3.x (49 apiece), not counting Windows machines that aren't running configure. We can't commit something that's going to make half the buildfarm go red. (It's likely that some fraction of them do already have python3 installed, in which case the search order change Peter recommended would be enough to fix it. But I'm sure not all do.) This ties into the business about converting the build system to meson, as that also requires python 3 --- with, IIUC, a higher minimum version than we might otherwise need. I'm disinclined to cause two separate flag days for buildfarm owners, so what I now think is we ought to put this idea on the shelf until we've finished that conversion or decided we're not gonna do it. We need to identify exactly what needs to be installed before we start pestering the owners. regards, tom lane
Re: enhance pg_log_backend_memory_contexts() to log memory contexts of auxiliary processes
On Fri, Nov 5, 2021 at 11:12 AM Bharath Rupireddy wrote: > PSA v2 patch and review it. I've modified the docs part a bit, please consider v3 for review. Regards, Bharath Rupireddy. v3-0001-enhance-pg_log_backend_memory_contexts-to-log-mem.patch Description: Binary data
Re: Printing backtrace of postgres processes
On Sun, Nov 14, 2021 at 8:49 PM vignesh C wrote: > > 7) Do we need TAP tests for this function? I think it is sufficient to > > test the function in misc_functions.sql, please remove > > 002_print_backtrace_validation.pl. Note that we don't do similar TAP > > testing for pg_log_backend_memory_contexts as well. > > I felt let's keep this test case, all the other tests just check if it > returns true or false, it does not checks for the contents in the > logfile. This is the only test which checks the logfile. I still don't agree to have test cases within a new file 002_print_backtrace_validation.pl. I feel this test case doesn't add value because the code coverage is done by .sql test cases and .pl just ensures the backtrace appears in the server logs. I don't think we ever need a new file for this purpose. If this is the case, then there are other functions like pg_log_backend_memory_contexts or pg_log_query_plan (in progress thread) might add the same test files for the same reasons which make the TAP tests i.e. "make check-world" to take longer times. Moreover, pg_log_backend_memory_contexts has been committed without having a TAP test case. I think we can remove it. Few more comments on v11: 1) I think we can improve here by adding a link to "backend" as well, I will modify it in the other thread. +Requests to log the backtrace of the backend or the +WAL sender or Something like: +Requests to log the backtrace of the backend or the +WAL sender or 2) I think "which is enough because the target process for logging of backtrace is a backend" isn't valid anymore with 0002, righit? Please remove it. + * to call this function if we see PrintBacktracePending set. It is called from + * CHECK_FOR_INTERRUPTS() or from process specific interrupt handlers, which is + * enough because the target process for logging of backtrace is a backend. > Thanks for the comments, v11 patch attached at [1] has the changes for the > same. The v11 patches mostly look good to me except the above comments. Regards, Bharath Rupireddy.
RE: [BUG]Invalidate relcache when setting REPLICA IDENTITY
On Sat, Nov 13, 2021 6:50 PM Amit Kapila wrote: > > The patch looks mostly good to me. I have slightly tweaked the comments in > the code (as per my previous suggestion) and test. Also, I have slightly > modified the commit message. If the attached looks good to you then kindly > prepare patches for back-branches. Thanks for reviewing, the latest patch looks good to me. Attach the patches for back branch (HEAD ~ v10). Best regards, Hou zj v8-HEAD-0001-Invalidate-relcache-when-changing-REPLICA-IDENTIT.patch Description: v8-HEAD-0001-Invalidate-relcache-when-changing-REPLICA-IDENTIT.patch v8-PG10-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch Description: v8-PG10-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch v8-PG12-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch Description: v8-PG12-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch v8-PG11-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch Description: v8-PG11-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch v8-PG13-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch Description: v8-PG13-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch v8-PG14-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch Description: v8-PG14-0001-Invalidate-relcache-when-changing-REPLICA-IDENTITY-i.patch
Add psql command to list constraints
Hi, I have been wondering why there is no meta-command for listing constraints in psql. So, I created a POC patch by using my experience developing \dX command in PG14. This feature is helpful for DBAs when they want to check or modify the definition of constraints. The current status of the POC patch is as follows: - Add "\dco" command to list constraints from pg_constraint - Not implemented yet: - NOT NULL constraint, and so on (based on pg_attribute) - Tab completion - Regression test - Document The following is test results (See attached test_list_con.sql) postgres=# \dco List of constsraints Schema | Name | Definition | Table +-+-+-- public | t01_chk_price_check | CHECK ((price > (0)::numeric)) | t01_chk public | t02_uniq_product_no_key | UNIQUE (product_no) | t02_uniq public | t03_pk1_pkey| PRIMARY KEY (product_no) | t03_pk1 public | t03_pk2_product_no_key | UNIQUE (product_no) | t03_pk2 public | t04_fk_pkey | PRIMARY KEY (order_id) | t04_fk public | t04_fk_product_no_fkey | FOREIGN KEY (product_no) REFERENCES t03_pk1(product_no) | t04_fk public | t05_ex_c_excl | EXCLUDE USING gist (c WITH &&) | t05_ex (7 rows) I have the following two questions that need to be discussed. Questions: (1) What strings should be assigned as meta-command for this feature? Currently, \dc and \dC are not available, so I tentatively assigned \dco. However, I do not have a strong opinion, so please let me know if you have any suggestions. (2) About domain constraints There is the \dD command to show a list of domain constraints. So I think this feature should not include it. Is it Okay? If I can get "+1" for this new feature development, I would like to improve the patch by adding NOT NULL constraints, and so on. Any advice or comments would be appreciated. Thanks, Tatsuro Yamada -- check CREATE TABLE t01_chk ( product_no integer, name text, price numeric CHECK (price > 0) ); -- unique CREATE TABLE t02_uniq ( product_no integer UNIQUE, name text, price numeric ); -- primary key CREATE TABLE t03_pk1 ( product_no integer PRIMARY KEY, name text, price numeric ); CREATE TABLE t03_pk2 ( product_no integer UNIQUE NOT NULL, name text, price numeric ); -- foreign key CREATE TABLE t04_fk ( order_id integer PRIMARY KEY, product_no integer REFERENCES t03_pk1 (product_no), quantity integer ); -- exclude CREATE TABLE t05_ex ( c circle, EXCLUDE USING gist (c WITH &&) ); -- trigger CREATE OR REPLACE FUNCTION trigger_notice_ab() RETURNS trigger as $$ begin raise notice 'trigger % on % % % for %: (a,b)=(%,%)', TG_NAME, TG_TABLE_NAME, TG_WHEN, TG_OP, TG_LEVEL, NEW.a, NEW.b; if TG_LEVEL = 'ROW' then return NEW; end if; return null; end; $$ language plpgsql; CREATE TABLE t06_trg (a int, b int); CREATE CONSTRAINT TRIGGER t06_con_trig AFTER insert ON t06_trg DEFERRABLE FOR each row EXECUTE PROCEDURE trigger_notice_ab(); -- domain CREATE DOMAIN t07_posint AS integer CHECK (VALUE > 0); CREATE TABLE t07_dm (id t07_posint); -- not null CREATE TABLE t08_notnull ( a varchar NOT NULL ); -- default CREATE TABLE t09_default ( a varchar DEFAULT 'a' ); -- Tests \dco \dco t01_chk_price_check \dco t02* \dcoS pg_stat* -- Visibility tests visible create schema con_s1; create TABLE con_s1.t10_test (a int PRIMARY KEY); set search_path to public, con_s1; \dco not visible create role regress_constraint nosuperuser; set role regress_constraint; \dco reset role; -- clean-up DROP TABLE t01_chk, t02_uniq, t03_pk1, t03_pk2, t04_fk, t05_ex, t06_trg, t07_dm, t08_notnull, t09_default, con_s1.t10_test; drop domain t07_posint; drop schema con_s1; drop role regress_constraint; diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 49d4c0e3ce..c450972f27 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -769,7 +769,10 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd) success = describeTablespaces(pattern, show_verbose); break; case 'c': - success = listConversions(pattern, show_verbose, show_system); + if (strncmp(cmd, "dco", 3) == 0 || strncmp(cmd, "dcoS", 4) == 0) /* Constraint */ +
Re: Skipping logical replication transactions on subscriber side
On Tue, Nov 9, 2021 at 3:07 PM Dilip Kumar wrote: > > On Sun, Nov 7, 2021 at 7:50 PM Masahiko Sawada wrote: > > I've attached an updated patch. In this version patch, subscription > > worker statistics are collected per-database and handled in a similar > > way to tables and functions. I think perhaps we still need to discuss > > details of how the statistics should be handled but I'd like to share > > the patch for discussion. > > While reviewing the v20, I have some initial comments, > > + > + > pg_stat_subscription_workerspg_stat_subscription_workers > + At least one row per subscription, showing about errors that > + occurred on subscription. > + See > + pg_stat_subscription_workers for > details. > + > > 1. > I don't like the fact that this view is very specific for showing the > errors but the name of the view is very generic. So are we keeping > this name to expand the scope of the view in the future? If this is > meant only for showing the errors then the name should be more > specific. As Amit already mentioned, we're planning to add more xact statistics to this view. I've mentioned that in the commit message. > > 2. > Why comment says "At least one row per subscription"? this looks > confusing, I mean if there is no error then there will not be even one > row right? > > > + > + The pg_stat_subscription_workers view will > contain > + one row per subscription error reported by workers applying logical > + replication changes and workers handling the initial data copy of the > + subscribed tables. > + Right. Fixed. > > 3. > So there will only be one row per subscription? I did not read the > code, but suppose there was an error due to some constraint now if > that constraint is removed and there is a new error then the old error > will be removed immediately or it will be removed by auto vacuum? If > it is not removed immediately then there could be multiple errors per > subscription in the view so the comment is not correct. There is one row per subscription worker (apply worker and tablesync worker). If the same error consecutively occurred, error_count is incremented and last_error_time is updated. Otherwise, i.g., if a different error occurred on the apply worker, all statistics are updated. > > 4. > + > + > + last_error_time timestamp > with time zone > + > + > + Time at which the last error occurred > + > + > > Will it be useful to know when the first time error occurred? Good idea. Users can know when the subscription stopped due to this error. Added. > > 5. > + > + > + stats_reset timestamp with > time zone > + > + > > The actual view does not contain this column. Removed. > > 6. > + > +Resets statistics of a single subscription worker statistics. > > /Resets statistics of a single subscription worker statistics/Resets > statistics of a single subscription worker Fixed. I'll update an updated patch soon. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Skipping logical replication transactions on subscriber side
On Mon, Nov 8, 2021 at 4:10 PM Greg Nancarrow wrote: > > On Mon, Nov 8, 2021 at 1:20 AM Masahiko Sawada wrote: > > > > I've attached an updated patch. In this version patch, subscription > > worker statistics are collected per-database and handled in a similar > > way to tables and functions. I think perhaps we still need to discuss > > details of how the statistics should be handled but I'd like to share > > the patch for discussion. > > > > That's for the updated patch. > Some initial comments on the v20 patch: Thank you for the comments! > > > doc/src/sgml/monitoring.sgml > > (1) wording > The word "information" seems to be missing after "showing" (otherwise > is reads "showing about errors", which isn't correct grammar). > I suggest the following change: > > BEFORE: > + At least one row per subscription, showing about errors that > + occurred on subscription. > AFTER: > + At least one row per subscription, showing information about > + errors that occurred on subscription. Fixed. > > (2) pg_stat_reset_subscription_worker(subid Oid, relid Oid) function > documentation > The description doesn't read well. I'd suggest the following change: > > BEFORE: > * Resets statistics of a single subscription worker statistics. > AFTER: > * Resets the statistics of a single subscription worker. > > I think that the documentation for this function should make it clear > that a non-NULL "subid" parameter is required for both reset cases > (tablesync and apply). > Perhaps this could be done by simply changing the first sentence to say: > "Resets the statistics of a single subscription worker, for a worker > running on the subscription with subid." > (and then can remove " running on the subscription with > subid" from the last sentence) Fixed. > > I think that the documentation for this function should say that it > should be used in conjunction with the "pg_stat_subscription_workers" > view in order to obtain the required subid/relid values for resetting. > (and should provide a link to the documentation for that view) I think it's not necessarily true that users should use pg_stat_subscription_workers in order to obtain subid/relid since we can obtain the same also from pg_subscription_rel. But I agree that it should clarify that this function resets entries of pg_stat_subscription view. Fixed. > > Also, I think that the function documentation should make it clear how > to distinguish the tablesync vs apply worker statistics case. > e.g. the tablesync error case is indicated by a null "command" in the > information returned from the "pg_stat_subscription_workers" view > (otherwise it seems a user could only know this by looking at the server log). The documentation of pg_stat_subscription_workers explains that subrelid is always NULL for apply workers. Is it not enough? > > Finally, there are currently no tests for this new function. I've added some tests. > > (3) pg_stat_subscription_workers > In the documentation for this, some users may not realise that "the > initial data copy" refers to "tablesync", so maybe say "the initial > data copy (tablesync)", or similar. > Perhaps it's better not to use the term "tablesync" since we don't use the term anywhere now. Instead, we should say more clearly, say "subscription worker handling initial data copy of the relation, as the description pg_stat_subscription says. > (4) stats_reset > "stats_reset" is currently documented as the last column of the > "pg_stat_subscription_workers" view - but it's actually no longer > included in the view. Removed. > > (5) src/tools/pgindent/typedefs.list > The following current entries are bogus: > PgStat_MsgSubWorkerErrorPurge > PgStat_MsgSubWorkerPurge > > The following entry is missing: > PgStat_MsgSubscriptionPurge Fixed. I'll submit an updated patch soon. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Emit a warning if the extension's GUC is set incorrectly
On 2021-11-15 04:50, Daniel Gustafsson wrote:
Seems reasonable on a quick skim, commit da2c1b8a2 did a similar
roundup back
in 2009 but at the time most of these didn't exist (pg_trgm did but
didn't have
custom option back then). There is one additional callsite defining
custom
variables in src/pl/tcl/pltcl.c which probably should get this
treatment as
well, it would align it with the pl/perl counterpart.
I'll have a closer look and test tomorrow.
Thank you for the review!
I have missed src/pl/tcl/pltcl.c, so I created the new patch.
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index 5820ac328d..d11dd1e416 100644
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -67,6 +67,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("auth_delay");
+
/* Install Hooks */
original_client_auth_hook = ClientAuthentication_hook;
ClientAuthentication_hook = auth_delay_checks;
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index fb38135f7a..0407c7dd64 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -100,6 +100,8 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("pg_trgm");
}
/*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 48c7417e6e..36555398ec 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -469,4 +469,6 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("postgres_fdw");
}
diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index 19a3ffb7ff..44a09c35a7 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -455,6 +455,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("sepgsql");
+
/* Initialize userspace access vector cache */
sepgsql_avc_init();
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index e11837559d..1389aa0943 100644
--- a/src/pl/tcl/pltcl.c
+++ b/src/pl/tcl/pltcl.c
@@ -474,6 +474,8 @@ _PG_init(void)
PGC_SUSET, 0,
NULL, NULL, NULL);
+ EmitWarningsOnPlaceholders("pltclu");
+
pltcl_pm_init_done = true;
}
Re: row filtering for logical replication
On Fri, Nov 12, 2021 at 9:19 PM Ajin Cherian wrote:
>
> Attaching version 39-
Here are some review comments for v39-0006:
1)
@@ -261,9 +261,9 @@ rowfilter_expr_replident_walker(Node *node,
rf_context *context)
* Rule 1. Walk the parse-tree and reject anything other than very simple
* expressions (See rowfilter_validator for details on what is permitted).
*
- * Rule 2. If the publish operation contains "delete" then only columns that
- * are allowed by the REPLICA IDENTITY rules are permitted to be used in the
- * row-filter WHERE clause.
+ * Rule 2. If the publish operation contains "delete" or "delete" then only
+ * columns that are allowed by the REPLICA IDENTITY rules are permitted to
+ * be used in the row-filter WHERE clause.
*/
static void
rowfilter_expr_checker(Publication *pub, ParseState *pstate, Node
*rfnode, Relation rel)
@@ -276,12 +276,10 @@ rowfilter_expr_checker(Publication *pub,
ParseState *pstate, Node *rfnode, Relat
rowfilter_validator(relname, rfnode);
/*
- * Rule 2: For "delete", check that filter cols are also valid replica
+ * Rule 2: For "delete" and "update", check that filter cols are also
valid replica
* identity cols.
- *
- * TODO - check later for publish "update" case.
*/
- if (pub->pubactions.pubdelete)
1a)
Typo - the function comment: "delete" or "delete"; should say:
"delete" or "update"
1b)
I felt it would be better (for the comment in the function body) to
write it as "or" instead of "and" because then it matches with the
code "if ||" that follows this comment.
2)
@@ -746,6 +780,92 @@ logicalrep_read_typ(StringInfo in, LogicalRepTyp *ltyp)
}
/*
+ * Write a tuple to the outputstream using cached slot, in the most
efficient format possible.
+ */
+static void
+logicalrep_write_tuple_cached(StringInfo out, Relation rel,
TupleTableSlot *slot, bool binary)
The function logicalrep_write_tuple_cached seems to have almost all of
its function body in common with logicalrep_write_tuple. Is there any
good way to combine these functions to avoid ~80 lines mostly
duplicated code?
3)
+ if (!old_matched && !new_matched)
+ return false;
+
+ if (old_matched && new_matched)
+ *action = REORDER_BUFFER_CHANGE_UPDATE;
+ else if (old_matched && !new_matched)
+ *action = REORDER_BUFFER_CHANGE_DELETE;
+ else if (new_matched && !old_matched)
+ *action = REORDER_BUFFER_CHANGE_INSERT;
+
+ return true;
I felt it is slightly confusing to have inconsistent ordering of the
old_matched and new_matched in those above conditions.
I suggest to use the order like:
* old-row (no match) new-row (no match)
* old-row (no match) new row (match)
* old-row (match) new-row (no match)
* old-row (match) new row (match)
And then be sure to keep consistent ordering in all places it is mentioned:
* in the code
* in the function header comment
* in the commit comment
* in docs?
4)
+/*
+ * Change is checked against the row filter, if any.
+ *
+ * If it returns true, the change is replicated, otherwise, it is not.
+ */
+static bool
+pgoutput_row_filter_virtual(Relation relation, TupleTableSlot *slot,
RelationSyncEntry *entry)
+{
+ EState*estate;
+ ExprContext *ecxt;
+ bool result = true;
+ Oid relid = RelationGetRelid(relation);
+
+ /* Bail out if there is no row filter */
+ if (!entry->exprstate)
+ return true;
+
+ elog(DEBUG3, "table \"%s.%s\" has row filter",
+ get_namespace_name(get_rel_namespace(relid)),
+ get_rel_name(relid));
It seems like that elog may consume unnecessary CPU most of the time.
I think it might be better to remove the relid declaration and rewrite
that elog as:
if (message_level_is_interesting(DEBUG3))
elog(DEBUG3, "table \"%s.%s\" has row filter",
get_namespace_name(get_rel_namespace(entry->relid)),
get_rel_name(entry->relid));
5)
diff --git a/src/include/replication/reorderbuffer.h
b/src/include/replication/reorderbuffer.h
index 5b40ff7..aec0059 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -51,7 +51,7 @@ typedef struct ReorderBufferTupleBuf
* respectively. They're used by INSERT .. ON CONFLICT .. UPDATE. Users of
* logical decoding don't have to care about these.
*/
-enum ReorderBufferChangeType
+typedef enum ReorderBufferChangeType
{
REORDER_BUFFER_CHANGE_INSERT,
REORDER_BUFFER_CHANGE_UPDATE,
@@ -65,7 +65,7 @@ enum ReorderBufferChangeType
REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM,
REORDER_BUFFER_CHANGE_INTERNAL_SPEC_ABORT,
REORDER_BUFFER_CHANGE_TRUNCATE
-};
+} ReorderBufferChangeType;
This new typedef can be added to src/tools/pgindent/typedefs.list.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: JIT doing duplicative optimization?
I wrote:
> You could probably generate some queries with lots and lots of expressions
> to characterize this better. If it is O(N^2), it should not be hard to
> drive the cost up to the point where the guilty bit of code would stand
> out in a perf trace.
I experimented with that, using a few different-size queries generated
like this:
print "explain analyze\n";
for (my $i = 1; $i < 100; $i++) {
print " select sum(f1+$i) from base union all\n";
}
print "select sum(f1+0) from base;\n";
on a table made like
create table base as select generate_series(1,1000) f1;
What I got, after setting max_parallel_workers_per_gather = 0,
was
10 subqueries:
Planning Time: 0.260 ms
JIT:
Functions: 30
Options: Inlining true, Optimization true, Expressions true, Deforming true
Timing: Generation 4.651 ms, Inlining 8.870 ms, Optimization 152.937 ms, Emis
sion 95.046 ms, Total 261.504 ms
Execution Time: 15258.249 ms
100 subqueries:
Planning Time: 2.231 ms
JIT:
Functions: 300
Options: Inlining true, Optimization true, Expressions true, Deforming true
Timing: Generation 44.163 ms, Inlining 9.934 ms, Optimization 1448.971 ms, Em
ission 928.438 ms, Total 2431.506 ms
Execution Time: 154815.515 ms
1000 subqueries:
Planning Time: 29.480 ms
JIT:
Functions: 3000
Options: Inlining true, Optimization true, Expressions true, Deforming true
Timing: Generation 444.479 ms, Inlining 25.688 ms, Optimization 14989.696 ms,
Emission 9891.993 ms, Total 25351.856 ms
Execution Time: 1522011.367 ms
So the overhead looks pretty linear, or even a shade sublinear for the
"inlining" bit, *as long as only one process is involved*. However,
I noted that if I didn't force that, the JIT overhead went up because
the planner wanted to use more workers and each worker has to do its own
compilations. So perhaps the apparent nonlinearity in your examples comes
from that?
BTW, I realized while working on this that I have little idea what the
"Functions:" count is. Nor does our documentation explain that (or any
other of these numbers), at least not anywhere I could find. That seems
like a pretty serious documentation fail. If these numbers aren't
important enough to explain in the docs, why are we printing them at all?
regards, tom lane
Re: Inconsistent error message for varchar(n)
Tom Lane writes: > Tracking it a bit further, the actual typmod limit is set by this: > > /* > * MaxAttrSize is a somewhat arbitrary upper limit on the declared size of > * data fields of char(n) and similar types. It need not have anything > * directly to do with the *actual* upper limit of varlena values, which > * is currently 1Gb (see TOAST structures in postgres.h). I've set it > * at 10Mb which seems like a reasonable number --- tgl 8/6/00. > */ > #define MaxAttrSize (10 * 1024 * 1024) > > So maybe that's something we *should* document, though we'd have to > explain that the limit on text and unconstrained varchar is different. > > regards, tom lane > > (From the writing style, I suspect the "tgl" here is me not Tom Lockhart. > I'm too lazy to dig in the git history to confirm it though.) I was bored, and found this: commit 022417740094620880488dd9b04fbb96ff11694b Author: Tom Lane Date: 2000-08-07 20:16:13 + TOAST mop-up work: update comments for tuple-size-related symbols such as MaxHeapAttributeNumber. Increase MaxAttrSize to something more reasonable (given what it's used for, namely checking char(n) declarations, I didn't make it the full 1G that it could theoretically be --- 10Mb seemed a more reasonable number). Improve calculation of MaxTupleSize. which added the above comment and changed MaxAttrSize: -#define MaxAttrSize(MaxTupleSize - MAXALIGN(sizeof(HeapTupleHeaderData))) +#define MaxAttrSize(10 * 1024 * 1024) - ilmari
Re: Atomic rename feature for Windows.
Hi
Added the pgunlink_windows_posix_semantics function and modified the
pgunlink function
I used FILE_DISPOSITION_POSIX_SEMANTICS flag for unlink files on Windows
10 (1607) and above.
Victor Spirin
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company
05.07.2021 16:53, Victor Spirin пишет:
Hi
I used the SetFileInformationByHandle function with the
FILE_RENAME_FLAG_POSIX_SEMANTICS flag for the file rename function..
1) The _WIN32_WINNT variable needs to be increased to 0x0A00 (Windows
10). Fixed conflict with #undef CHECKSUM_TYPE_NONE
2) The SetFileInformationByHandle function works correctly only on
Windows 10 and higher.
The app must have a manifest to check the Windows version using the
IsWindows10OrGreater() function. I added a manifest to all postgres
projects and disabled the GenerateManifest option on windows projects.
This patch related to this post:
https://www.postgresql.org/message-id/CAEepm%3D0FV-k%2B%3Dd9z08cW%3DZXoR1%3Dkw9wdpkP6WAuOrKJdz-8ujg%40mail.gmail.com
diff --git a/src/include/common/checksum_helper.h
b/src/include/common/checksum_helper.h
index cac7570ea1..2d533c93a6 100644
--- a/src/include/common/checksum_helper.h
+++ b/src/include/common/checksum_helper.h
@@ -26,6 +26,13 @@
* MD5 here because any new that does need a cryptographically strong checksum
* should use something better.
*/
+
+ /*
+ * CHECKSUM_TYPE_NONE defined in the winioctl.h when _WIN32_WINNT >=
_WIN32_WINNT_WIN10
+ */
+#ifdef CHECKSUM_TYPE_NONE
+#undef CHECKSUM_TYPE_NONE
+#endif
typedef enum pg_checksum_type
{
CHECKSUM_TYPE_NONE,
diff --git a/src/include/port/win32.h b/src/include/port/win32.h
index d8ae49e22d..d91555f5c0 100644
--- a/src/include/port/win32.h
+++ b/src/include/port/win32.h
@@ -12,12 +12,13 @@
/*
* Make sure _WIN32_WINNT has the minimum required value.
* Leave a higher value in place. When building with at least Visual
- * Studio 2015 the minimum requirement is Windows Vista (0x0600) to
- * get support for GetLocaleInfoEx() with locales. For everything else
+ * Studio 2015 the minimum requirement is Windows 10 (0x0A00) to get support
for SetFileInformationByHandle.
+ * The minimum requirement is Windows Vista (0x0600) get support for
GetLocaleInfoEx() with locales.
+ * For everything else
* the minimum version is Windows XP (0x0501).
*/
#if defined(_MSC_VER) && _MSC_VER >= 1900
-#define MIN_WINNT 0x0600
+#define MIN_WINNT 0x0A00
#else
#define MIN_WINNT 0x0501
#endif
diff --git a/src/port/dirmod.c b/src/port/dirmod.c
index 763bc5f915..6798b83908 100644
--- a/src/port/dirmod.c
+++ b/src/port/dirmod.c
@@ -39,6 +39,197 @@
#endif
#endif
+
+#if defined(WIN32) && !defined(__CYGWIN__) && defined(_WIN32_WINNT_WIN10) &&
_WIN32_WINNT >= _WIN32_WINNT_WIN10
+
+#include
+
+/*
+ * Checks Windows version using RtlGetVersion
+ * Version 1607 (Build 14393) is required for SetFileInformationByHandle
+ * function with FILE_RENAME_FLAG_POSIX_SEMANTICS flag
+*/
+typedef NTSYSAPI(NTAPI * PFN_RTLGETVERSION)
+(OUT PRTL_OSVERSIONINFOEXW lpVersionInformation);
+
+static int isWin1607 = -1;
+static int isWindows1607OrGreater()
+{
+ HMODULE ntdll;
+ PFN_RTLGETVERSION _RtlGetVersion = NULL;
+ OSVERSIONINFOEXW info;
+ if (isWin1607 >= 0) return isWin1607;
+ ntdll = LoadLibraryEx("ntdll.dll", NULL, 0);
+ if (ntdll == NULL)
+ {
+ DWORD err = GetLastError();
+
+ _dosmaperr(err);
+ return -1;
+ }
+
+ _RtlGetVersion = (PFN_RTLGETVERSION)(pg_funcptr_t)
+ GetProcAddress(ntdll, "RtlGetVersion");
+ if (_RtlGetVersion == NULL)
+ {
+ DWORD err = GetLastError();
+
+ FreeLibrary(ntdll);
+ _dosmaperr(err);
+ return -1;
+ }
+ info.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEXW);
+ if (!NT_SUCCESS(_RtlGetVersion()))
+ {
+ DWORD err = GetLastError();
+
+ FreeLibrary(ntdll);
+ _dosmaperr(err);
+ return -1;
+ }
+
+ if (info.dwMajorVersion >= 10 && info.dwBuildNumber >= 14393) isWin1607
= 1;
+ else isWin1607 = 0;
+ FreeLibrary(ntdll);
+ return isWin1607;
+
+}
+
+typedef struct _FILE_RENAME_INFO_EXT {
+ FILE_RENAME_INFO fri;
+ WCHAR extra_space[MAX_PATH];
+} FILE_RENAME_INFO_EXT;
+
+/*
+ * pgrename_windows_posix_semantics - uses SetFileInformationByHandle function
+ * with FILE_RENAME_FLAG_POSIX_SEMANTICS flag for atomic rename file
+ * working only on Windows 10 (1607) or later and _WIN32_WINNT must be >=
_WIN32_WINNT_WIN10
+ */
+static int
+pgrename_windows_posix_semantics(const char *from, const char *to)
+{
+ int err;
+ FILE_RENAME_INFO_EXT rename_info;
+ PFILE_RENAME_INFO prename_info;
+ HANDLE f_handle;
+ wchar_t from_w[MAX_PATH];
+ DWORD dwFlagsAndAttributes;
+ DWORD dwAttrib;
+
+
+
Re: Emit a warning if the extension's GUC is set incorrectly
> On 14 Nov 2021, at 11:03, Shinya Kato wrote: > If wrong GUCs of auth_delay, pg_trgm, postgres_fdw and sepgsql are described > in postgresql.conf, a warning is not emitted unlike pg_stat_statements, > auto_explain and pg_prewarm. > So, I improved it by adding EmitWarningsOnPlaceholders. > An example output is shown below. > --- > 2021-11-14 18:18:16.486 JST [487067] WARNING: unrecognized configuration > parameter "auth_delay.xxx" > --- > > What do you think? Seems reasonable on a quick skim, commit da2c1b8a2 did a similar roundup back in 2009 but at the time most of these didn't exist (pg_trgm did but didn't have custom option back then). There is one additional callsite defining custom variables in src/pl/tcl/pltcl.c which probably should get this treatment as well, it would align it with the pl/perl counterpart. I'll have a closer look and test tomorrow. -- Daniel Gustafsson https://vmware.com/
Re: Commitfest 2021-11 Patch Triage - Part 2
Stephen Frost writes: > Attackers aren't likely to have the kind of isolated control over the > data in the WAL stream (which is a combination of data from lots of > ongoing activity in the system and isn't likely to be exactly what the > attacker supplied at some higher level anyway) and the ability to read > and analyze the WAL stream from a primary to a replica to be able to > effectively attack it. Yeah, I concur with that so far as WAL data goes. A hypothetical attacker will not have control over xact IDs, tuple TIDs, etc, which will add enough entropy to the stream that extracting data payloads seems pretty infeasible. My concern upthread was about client-session connections, where such mitigation doesn't apply. (I wonder a bit about logical-replication streams, too.) regards, tom lane
Re: Commitfest 2021-11 Patch Triage - Part 2
Greetings, * Andrey Borodin (x4...@yandex-team.ru) wrote: > > On 11/10/21 16:54, Andrey Borodin wrote: > >> Compression is crucial for highly available setups. Replication traffic is > >> often billed. Or route has bandwidth limits. > >> An entropy added by WAL headers makes CRIME attack against replication > >> encryption impractical. > > > > I very much doubt WAL headers are a reliable protection against CRIME, > > because the entropy of the headers is likely fairly constant. So if you > > compress the WAL stream, the WAL headers may change but the compression > > ratio should be pretty similar. At least that's my guess. > > I've thought more about it and I agree. > To reliably protect against CRIME entropy of WAL headers must be comparable > with the entropy of possibly injected data. > If this would stand, probably, our WAL would need a really serious rework. > > Maybe just refuse to enable compression on SSL connection? If someone really > needs both - they will just patch a server on their own. > Or make a GUC "yes_i_kwow_what_crime_is_give_grant_read_on_my_data_to_spies". Attackers aren't likely to have the kind of isolated control over the data in the WAL stream (which is a combination of data from lots of ongoing activity in the system and isn't likely to be exactly what the attacker supplied at some higher level anyway) and the ability to read and analyze the WAL stream from a primary to a replica to be able to effectively attack it. None of this kind of babysitting makes any sense to me. I'm certainly fine with documenting that there are cases where compression of data sent by an attacker and then sent over an encrypted channel has been able to break the encryption and let admins decide if it's appropriate or not for them to use in their environment. Thanks, Stephen signature.asc Description: PGP signature
Re: Inconsistent error message for varchar(n)
I wrote: > For comparison, it doesn't bring up the point that string values are > constrained to 1GB; that's dealt with elsewhere. Since the limit on > typmod is substantially more than that, I'm not sure there's much point > in mentioning it specifically. Oh, wait, I was not counting the zeroes in that number :-( Tracking it a bit further, the actual typmod limit is set by this: /* * MaxAttrSize is a somewhat arbitrary upper limit on the declared size of * data fields of char(n) and similar types. It need not have anything * directly to do with the *actual* upper limit of varlena values, which * is currently 1Gb (see TOAST structures in postgres.h). I've set it * at 10Mb which seems like a reasonable number --- tgl 8/6/00. */ #define MaxAttrSize (10 * 1024 * 1024) So maybe that's something we *should* document, though we'd have to explain that the limit on text and unconstrained varchar is different. regards, tom lane (From the writing style, I suspect the "tgl" here is me not Tom Lockhart. I'm too lazy to dig in the git history to confirm it though.)
Re: Inconsistent error message for varchar(n)
[ Please trim quotes appropriately when replying. Nobody wants to read the whole history of the thread to get to your comment. ] Japin Li writes: > Oh! I didn't consider this situation. Since the max size of varchar cannot > exceed 10485760, however, I cannot find this in documentation [1]. Is there > something I missed? Should we mention this in the documentation? > [1] https://www.postgresql.org/docs/devel/datatype-character.html I dunno, that section doesn't really get into implementation limits. For comparison, it doesn't bring up the point that string values are constrained to 1GB; that's dealt with elsewhere. Since the limit on typmod is substantially more than that, I'm not sure there's much point in mentioning it specifically. Maybe there's a case for mentioning the 1GB limit here, though. regards, tom lane
Re: JIT doing duplicative optimization?
Alvaro Herrera writes: > On 2021-Nov-11, Alvaro Herrera wrote: >> But what really surprised me is that the the average time to optimize >> per function is now 2.06ms ... less than half of the previous >> measurement. It emits 10% less functions than before, but the time to >> both optimize and emit is reduced by 50%. How does that make sense? > Ah, here's a query that illustrates what I'm on about. I found this > query[1] in a blog post[2]. > ... > Query 1, 148 functions JIT-compiled. > Average time to optimize, per function 435.153/148 = 2.940ms; > average time to emit per function 282.216/148 = 1.906ms > Query 2, 137 functions JIT-compiled. > Average time to optimize, per function: 374.103/137 = 2.730ms > Average time to emit, per function 254.557 / 137 = 1.858ms > Query 3, 126 functions JIT-compiled. > Average time to optimize per function 229.128 / 126 = 1.181ms > Average time to emit per function 167.338 / 126 = 1.328ms Yeah, in combination with your other measurement, it sure does look like there's something worse-than-linear going on here. The alternative is to assume that the individual functions are more complex in one query than the other, and that seems like a bit of a stretch. You could probably generate some queries with lots and lots of expressions to characterize this better. If it is O(N^2), it should not be hard to drive the cost up to the point where the guilty bit of code would stand out in a perf trace. regards, tom lane
Re: JIT doing duplicative optimization?
On Sun, Nov 14, 2021 at 9:07 AM Alvaro Herrera wrote: > On 2021-Nov-11, Alvaro Herrera wrote: > > > But what really surprised me is that the the average time to optimize > > per function is now 2.06ms ... less than half of the previous > > measurement. It emits 10% less functions than before, but the time to > > both optimize and emit is reduced by 50%. How does that make sense? > > Ah, here's a query that illustrates what I'm on about. I found this > query[1] in a blog post[2]. > > ``` > WITH RECURSIVE typeinfo_tree( > oid, ns, name, kind, basetype, elemtype, elemdelim, > range_subtype, attrtypoids, attrnames, depth) > AS ( > SELECT > ti.oid, ti.ns, ti.name, ti.kind, ti.basetype, > ti.elemtype, ti.elemdelim, ti.range_subtype, > ti.attrtypoids, ti.attrnames, 0 > FROM > ( > SELECT > t.oid AS oid, > ns.nspname AS ns, > t.typname AS name, > t.typtype AS kind, > (CASE WHEN t.typtype = 'd' THEN > (WITH RECURSIVE typebases(oid, depth) AS ( > SELECT > t2.typbasetype AS oid, > 0 AS depth > FROM > pg_type t2 > WHERE > t2.oid = t.oid > UNION ALL > SELECT > t2.typbasetype AS oid, > tb.depth + 1AS depth > FROM > pg_type t2, > typebases tb > WHERE >tb.oid = t2.oid >AND t2.typbasetype != 0 >) SELECT oid FROM typebases ORDER BY depth DESC LIMIT 1) >ELSE NULL > END)AS basetype, > t.typelem AS elemtype, > elem_t.typdelim AS elemdelim, > range_t.rngsubtype AS range_subtype, > (CASE WHEN t.typtype = 'c' THEN > (SELECT > array_agg(ia.atttypid ORDER BY ia.attnum) > FROM > pg_attribute ia > INNER JOIN pg_class c > ON (ia.attrelid = c.oid) > WHERE > ia.attnum > 0 AND NOT ia.attisdropped > AND c.reltype = t.oid) > ELSE NULL > END)AS attrtypoids, > (CASE WHEN t.typtype = 'c' THEN > (SELECT > array_agg(ia.attname::text ORDER BY ia.attnum) > FROM > pg_attribute ia > INNER JOIN pg_class c > ON (ia.attrelid = c.oid) > WHERE > ia.attnum > 0 AND NOT ia.attisdropped > AND c.reltype = t.oid) > ELSE NULL > END)AS attrnames > FROM > pg_catalog.pg_type AS t > INNER JOIN pg_catalog.pg_namespace ns ON ( > ns.oid = t.typnamespace) > LEFT JOIN pg_type elem_t ON ( > t.typlen = -1 AND > t.typelem != 0 AND > t.typelem = elem_t.oid > ) > LEFT JOIN pg_range range_t ON ( > t.oid = range_t.rngtypid > ) > ) AS ti > WHERE > ti.oid = any(ARRAY[16,17]::oid[]) > > UNION ALL > > SELECT > ti.oid, ti.ns, ti.name, ti.kind, ti.basetype, > ti.elemtype, ti.elemdelim, ti.range_subtype, > ti.attrtypoids, ti.attrnames, tt.depth + 1 > FROM > ( > SELECT > t.oid AS oid, > ns.nspname AS ns, > t.typname AS name, > t.typtype AS kind, > (CASE WHEN t.typtype = 'd' THEN > (WITH RECURSIVE typebases(oid, depth) AS ( > SELECT > t2.typbasetype AS oid, > 0 AS depth > FROM > pg_type t2 > WHERE > t2.oid = t.oid > UNION ALL > SELECT > t2.typbasetype AS oid, > tb.depth + 1AS depth > FROM > pg_type t2, > typebases tb > WHERE >tb.oid = t2.oid >AND t2.typbasetype != 0 >) SELECT oid FROM typebases ORDER BY depth DESC
Re: JIT doing duplicative optimization?
On 2021-Nov-11, Alvaro Herrera wrote: > But what really surprised me is that the the average time to optimize > per function is now 2.06ms ... less than half of the previous > measurement. It emits 10% less functions than before, but the time to > both optimize and emit is reduced by 50%. How does that make sense? Ah, here's a query that illustrates what I'm on about. I found this query[1] in a blog post[2]. ``` WITH RECURSIVE typeinfo_tree( oid, ns, name, kind, basetype, elemtype, elemdelim, range_subtype, attrtypoids, attrnames, depth) AS ( SELECT ti.oid, ti.ns, ti.name, ti.kind, ti.basetype, ti.elemtype, ti.elemdelim, ti.range_subtype, ti.attrtypoids, ti.attrnames, 0 FROM ( SELECT t.oid AS oid, ns.nspname AS ns, t.typname AS name, t.typtype AS kind, (CASE WHEN t.typtype = 'd' THEN (WITH RECURSIVE typebases(oid, depth) AS ( SELECT t2.typbasetype AS oid, 0 AS depth FROM pg_type t2 WHERE t2.oid = t.oid UNION ALL SELECT t2.typbasetype AS oid, tb.depth + 1AS depth FROM pg_type t2, typebases tb WHERE tb.oid = t2.oid AND t2.typbasetype != 0 ) SELECT oid FROM typebases ORDER BY depth DESC LIMIT 1) ELSE NULL END)AS basetype, t.typelem AS elemtype, elem_t.typdelim AS elemdelim, range_t.rngsubtype AS range_subtype, (CASE WHEN t.typtype = 'c' THEN (SELECT array_agg(ia.atttypid ORDER BY ia.attnum) FROM pg_attribute ia INNER JOIN pg_class c ON (ia.attrelid = c.oid) WHERE ia.attnum > 0 AND NOT ia.attisdropped AND c.reltype = t.oid) ELSE NULL END)AS attrtypoids, (CASE WHEN t.typtype = 'c' THEN (SELECT array_agg(ia.attname::text ORDER BY ia.attnum) FROM pg_attribute ia INNER JOIN pg_class c ON (ia.attrelid = c.oid) WHERE ia.attnum > 0 AND NOT ia.attisdropped AND c.reltype = t.oid) ELSE NULL END)AS attrnames FROM pg_catalog.pg_type AS t INNER JOIN pg_catalog.pg_namespace ns ON ( ns.oid = t.typnamespace) LEFT JOIN pg_type elem_t ON ( t.typlen = -1 AND t.typelem != 0 AND t.typelem = elem_t.oid ) LEFT JOIN pg_range range_t ON ( t.oid = range_t.rngtypid ) ) AS ti WHERE ti.oid = any(ARRAY[16,17]::oid[]) UNION ALL SELECT ti.oid, ti.ns, ti.name, ti.kind, ti.basetype, ti.elemtype, ti.elemdelim, ti.range_subtype, ti.attrtypoids, ti.attrnames, tt.depth + 1 FROM ( SELECT t.oid AS oid, ns.nspname AS ns, t.typname AS name, t.typtype AS kind, (CASE WHEN t.typtype = 'd' THEN (WITH RECURSIVE typebases(oid, depth) AS ( SELECT t2.typbasetype AS oid, 0 AS depth FROM pg_type t2 WHERE t2.oid = t.oid UNION ALL SELECT t2.typbasetype AS oid, tb.depth + 1AS depth FROM pg_type t2, typebases tb WHERE tb.oid = t2.oid AND t2.typbasetype != 0 ) SELECT oid FROM typebases ORDER BY depth DESC LIMIT 1) ELSE NULL END)AS basetype, t.typelem AS elemtype, elem_t.typdelim AS elemdelim, range_t.rngsubtype AS range_subtype, (CASE WHEN t.typtype
Re: Printing backtrace of postgres processes
On Fri, Nov 12, 2021 at 6:11 PM Bharath Rupireddy wrote: > > On Fri, Nov 12, 2021 at 5:15 PM Bharath Rupireddy > wrote: > > > > On Thu, Nov 11, 2021 at 12:14 PM vignesh C wrote: > > > Thanks for the comments, the attached v10 patch has the fixes for the > > > same. > > > > Thanks for the patches. Here are some comments: > > > > 1) In the docs, let's have the similar description of > > pg_log_backend_memory_contexts for pg_print_backtrace, just for the > > continuity in the users readability. > > > > 2) I don't know how the part looks like in the Server > > Signaling Functions table. I think here you can just say, it will emit > > a warning and return false if not supported by the installation. And > > you can give the part after the description where you are > > showing a sample backtrace. > > > > +capture backtrace. If not available, the function will return false > > +and a warning is issued, for example: > > + > > +WARNING: backtrace generation is not supported by this installation > > + pg_print_backtrace > > + > > + f > > + > > + > > + > > > > 3) Replace '!' with '.'. > > + * Note: this is called within a signal handler! All we can do is set > > > > 4) It is not only the next CFI but also the process specific interrupt > > handlers (in your 0002 patch) right? > > + * a flag that will cause the next CHECK_FOR_INTERRUPTS to invoke > > > > 5) I think you need to update CATALOG_VERSION_NO, mostly the committer > > will take care of it but just in case. > > > > 6) Be consistent with casing "Verify" and "Might" > > +# Verify that log output gets to the file > > +# might need to retry if logging collector process is slow... > > Few more comments: > > 7) Do we need TAP tests for this function? I think it is sufficient to > test the function in misc_functions.sql, please remove > 002_print_backtrace_validation.pl. Note that we don't do similar TAP > testing for pg_log_backend_memory_contexts as well. I felt let's keep this test case, all the other tests just check if it returns true or false, it does not checks for the contents in the logfile. This is the only test which checks the logfile. > 8) I hope you have manually tested the pg_print_backtrace for the > startup process and other auxiliary processes. Yes, I have checked them manually. > 9) I think we can have a similar description (to the patch [1]). with > links to each process definition in the glossary so that it will be > easier for the users to follow the links and know what each process > is. Especially, I didn't like the 0002 mentioned about the > BackendPidGetProc or AuxiliaryPidGetProc as these are internal to the > server and the users don't care about them. > > - * end on its own first and its backtrace are not logged is not a problem. > + * BackendPidGetProc or AuxiliaryPidGetProc returns NULL if the pid isn't > + * valid; but by the time we reach kill(), a process for which we get a > + * valid proc here might have terminated on its own. There's no way to > + * acquire a lock on an arbitrary process to prevent that. But since this > + * mechanism is usually used to debug a backend running and consuming lots > + * of CPU cycles, that it might end on its own first and its backtrace are > + * not logged is not a problem. > */ > > Here's what I have written in the other patch [1], if okay please use this: > > +Requests to log memory contexts of the backend or the > +WAL sender or > +the auxiliary > process > +with the specified process ID. All of the > +auxiliary > processes > +are supported except the linkend="glossary-logger">logger > +and the linkend="glossary-stats-collector">statistics collector > +as they are not connected to shared memory the function can > not make requests. > +These memory contexts will be logged at > LOG message level. > +They will appear in the server log based on the log configuration set > (See for more information), I had mentioned BackendPidGetProc or AuxiliaryPidGetProc as comments in the function, but I have not changed that. I have changed the documentation similar to your patch. Thanks for the comments, v11 patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm3BYGOG3-PQvYbWkB%3DG3h1KYJ8CO8UYbzfECH4DYGMGqA%40mail.gmail.com Regards, Vignesh
Re: Printing backtrace of postgres processes
On Fri, Nov 12, 2021 at 5:15 PM Bharath Rupireddy
wrote:
>
> On Thu, Nov 11, 2021 at 12:14 PM vignesh C wrote:
> > Thanks for the comments, the attached v10 patch has the fixes for the same.
>
> Thanks for the patches. Here are some comments:
>
> 1) In the docs, let's have the similar description of
> pg_log_backend_memory_contexts for pg_print_backtrace, just for the
> continuity in the users readability.
I have kept some contents of the description similar. There is some
additional information to explain more about the functionality. I felt
that will help the user to understand more about the feature.
> 2) I don't know how the part looks like in the Server
> Signaling Functions table. I think here you can just say, it will emit
> a warning and return false if not supported by the installation. And
> you can give the part after the description where you are
> showing a sample backtrace.
>
> +capture backtrace. If not available, the function will return false
> +and a warning is issued, for example:
> +
> +WARNING: backtrace generation is not supported by this installation
> + pg_print_backtrace
> +
> + f
> +
> +
> +
Modified
> 3) Replace '!' with '.'.
> + * Note: this is called within a signal handler! All we can do is set
I have changed it similar to HandleLogMemoryContextInterrupt
> 4) It is not only the next CFI but also the process specific interrupt
> handlers (in your 0002 patch) right?
> + * a flag that will cause the next CHECK_FOR_INTERRUPTS to invoke
Modified
> 5) I think you need to update CATALOG_VERSION_NO, mostly the committer
> will take care of it but just in case.
Modified
> 6) Be consistent with casing "Verify" and "Might"
> +# Verify that log output gets to the file
> +# might need to retry if logging collector process is slow...
Modified
The attached v11 patch has the changes for the same.
Regards,
Vignesh
From c29bafb470a367bd4f790f6f50edc3e8002f46c7 Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Tue, 9 Nov 2021 16:28:03 +0530
Subject: [PATCH v11 1/2] Print backtrace of specified postgres process.
The idea here is to implement & expose pg_print_backtrace function, internally
what this function does is, the connected backend will send SIGUSR1 signal by
setting PROCSIG_PRINT_BACKTRACE to postgres backend whose pid matches the
specified process id. Once the backend process receives this signal it will
print the backtrace of the process to the log file based on the logging
configuration, if logging is disabled backtrace will be printed to the
console where postmaster was started.
---
doc/src/sgml/func.sgml| 82 +++
src/backend/catalog/system_functions.sql | 2 +
src/backend/postmaster/autovacuum.c | 4 +
src/backend/storage/ipc/procsignal.c | 41 ++
src/backend/storage/ipc/signalfuncs.c | 48 +++
src/backend/tcop/postgres.c | 4 +
src/backend/utils/error/elog.c| 20 -
src/backend/utils/init/globals.c | 1 +
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.dat | 5 ++
src/include/miscadmin.h | 1 +
src/include/storage/procsignal.h | 3 +-
src/include/utils/elog.h | 2 +
.../t/002_print_backtrace_validation.pl | 73 +
src/test/regress/expected/misc_functions.out | 42 ++
src/test/regress/sql/misc_functions.sql | 31 +++
16 files changed, 355 insertions(+), 6 deletions(-)
create mode 100644 src/test/modules/test_misc/t/002_print_backtrace_validation.pl
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 24447c0017..620e6ee54a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25345,6 +25345,31 @@ SELECT collation for ('foo' COLLATE "de_DE");
+
+
+
+ pg_print_backtrace
+
+pg_print_backtrace ( pid integer )
+boolean
+
+
+Requests to log the backtrace of the backend with the specified process
+ID. The backtrace will be logged at message level
+LOG. It will appear in the server log based on the
+log configuration set (See for
+more information), but will not be sent to the client regardless of
+. A backtrace will identify
+where exactly the backend process is currently executing. This may be
+useful to developers to diagnose stuck processes and other problems.
+This feature is not supported for the postmaster, logger, checkpointer,
+walwriter, background writer or statistics collector process. This
+feature will be available if PostgreSQL was built with the ability to
+capture backtrace. If not available, the function will emit a warning
+and return false.
+
+
+
@@ -25458,6
Re: Logical Replication - improve error message while adding tables to the publication in check_publication_add_relation
On Sat, Nov 13, 2021 at 7:57 PM Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote: > > On Sat, Nov 13, 2021 at 7:16 PM Euler Taveira wrote: > > Thanks. It is a good idea to use errdetail_relkind_not_supported. I > > slightly modified the API to "int errdetail_relkind_not_supported(Oid > > relid, Form_pg_class rd_rel);" to simplify things and increase the > > usability of the function further. For instance, it can report the > > specific error for the catalog tables as well. And, also added "int > > errdetail_relkind_not_supported _v2(Oid relid, char relkind, char > > relpersistence);" so that the callers not having Form_pg_class (there > > are 3 callers exist) can pass the parameters directly. > > > > Do we really need 2 functions? I don't think so. Besides that, relid is > > redundant since this information is available in the Form_pg_class struct. > > Yeah. The relid is available in Form_pg_class. > > Firstly, I didn't quite like the function > errdetail_relkind_not_supported name to be too long here and adding to > it the 2 or 3 parameters, in many places we are crossing 80 char > limit. Above these, a function with one parameter is always better > than function with 3 parameters. > > Having two functions isn't a big deal at all, I think we have many > functions like that in the core (although I'm not gonna spend time > finding all those functions, I'm sure there will be such functions). > > I would still go with with 2 functions: > > int errdetail_relkind_not_supported(Form_pg_class rd_rel); > int errdetail_relkind_not_supported_v2(Oid relid, char relkind, char > relpersistence); > > > int errdetail_relkind_not_supported(Oid relid, Form_pg_class rd_rel); > > > > My suggestion is to keep only the 3 parameter function: > > > > int errdetail_relkind_not_supported(Oid relid, char relkind, char relpersistence); > > > > Multiple functions that is just a wrapper for a central one is a good idea for > > backward compatibility. That's not the case here. > > Since we are modifying it on the master, I think it is okay to have 2 > functions given the code simplification advantages we get with > errdetail_relkind_not_supported(Form_pg_class rd_rel). > > I would even think further to rename "errdetail_relkind_not_supported" > and have the following, because we don't have to be always descriptive > in the function names. The errdetail would tell the function is going > to give some error detail. > > int errdetail_relkind(Form_pg_class rd_rel); > int errdetail_relkind_v2(Oid relid, char relkind, char relpersistence); > > or > > int errdetail_rel(Form_pg_class rd_rel); > int errdetail_rel_v2(Oid relid, char relkind, char relpersistence); > > I prefer the above among the three function names. > > Thoughts? PSA v11 patch with 2 APIs with much simpler parameters and small function names: int errdetail_rel(Form_pg_class rd_rel); int errdetail_rel_v2(Oid relid, char relkind, char relpersistence); Please review it. Regards, Bharath Rupireddy. From 16876bdee3dccf6ec5c2faeb93dd3c51b62dbc2e Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Sun, 14 Nov 2021 03:57:11 + Subject: [PATCH v11] Improve publication error messages Provide accurate messages for CREATE PUBLICATION commands. Also add tests to cover temporary, unlogged, system and foreign tables. While at it, modify errdetail_relkind_not_supported API to distinguish tables based on persistence property and report for system tables. As the function name is too long rename it to errdetail_rel. Also, introduce errdetail_rel so that the callers not having Form_pg_class object can send relid, relkind, relpersistence as direct parameters. This way, the API is more flexbile and easier to use. It also provides accurate detail messages for future work. --- contrib/amcheck/verify_heapam.c | 2 +- contrib/pageinspect/rawpage.c | 2 +- contrib/pg_surgery/heap_surgery.c | 2 +- contrib/pg_visibility/pg_visibility.c | 2 +- contrib/pgstattuple/pgstatapprox.c| 2 +- contrib/pgstattuple/pgstatindex.c | 2 +- contrib/pgstattuple/pgstattuple.c | 2 +- .../postgres_fdw/expected/postgres_fdw.out| 6 contrib/postgres_fdw/sql/postgres_fdw.sql | 5 src/backend/catalog/pg_class.c| 25 +++-- src/backend/catalog/pg_publication.c | 28 ++- src/backend/commands/comment.c| 2 +- src/backend/commands/indexcmds.c | 2 +- src/backend/commands/lockcmds.c | 5 ++-- src/backend/commands/seclabel.c | 2 +- src/backend/commands/sequence.c | 2 +- src/backend/commands/statscmds.c | 2 +- src/backend/commands/subscriptioncmds.c | 4 +-- src/backend/commands/tablecmds.c | 9 +++--- src/backend/commands/trigger.c| 6 ++-- src/backend/executor/execReplication.c| 5 ++--
Re: add recovery, backup, archive, streaming etc. activity messages to server logs along with ps display
On Sat, Nov 13, 2021 at 9:04 PM Alvaro Herrera
wrote:
>
> A bunch of these now execute snprintf()s unnecessarily. I think these
> should be made conditional on message_level_is_interesting(DEBUG1) to
> avoid that overhead.
Thanks. Attaching the v2 to avoid that by directly using the message in
ereport instead of activitymsg.
Regards,
Bharath Rupireddy.
From 6eea044a734fc53353deaa973df6f70096160435 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Sun, 14 Nov 2021 02:29:04 +
Subject: [PATCH v2] add log messages equivalents of ps display activitymsgs
---
src/backend/access/transam/xlog.c | 6 ++
src/backend/postmaster/pgarch.c | 8
src/backend/replication/basebackup.c | 3 +++
src/backend/replication/walreceiver.c | 8
src/backend/replication/walsender.c | 4
5 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e073121a7e..3a62b4c745 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3791,6 +3791,9 @@ XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
xlogfname);
set_ps_display(activitymsg);
+ ereport(DEBUG1,
+ (errmsg_internal("waiting for %s", xlogfname)));
+
if (!RestoreArchivedFile(path, xlogfname,
"RECOVERYXLOG",
wal_segment_size,
@@ -3833,6 +3836,9 @@ XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
xlogfname);
set_ps_display(activitymsg);
+ ereport(DEBUG1,
+(errmsg_internal("recovering %s", xlogfname)));
+
/* Track source of data in assorted state variables */
readSource = source;
XLogReceiptSource = source;
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 3b33e01d95..abfe451dbe 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -547,14 +547,14 @@ pgarch_archiveXlog(char *xlog)
}
*dp = '\0';
- ereport(DEBUG3,
- (errmsg_internal("executing archive command \"%s\"",
- xlogarchcmd)));
-
/* Report archive activity in PS display */
snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
set_ps_display(activitymsg);
+ ereport(DEBUG1,
+ (errmsg_internal("executing archive command \"%s\" for archving %s",
+ xlogarchcmd, xlog)));
+
rc = system(xlogarchcmd);
if (rc != 0)
{
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index ec0485705d..bc173bfe8e 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -881,6 +881,9 @@ SendBaseBackup(BaseBackupCmd *cmd)
set_ps_display(activitymsg);
}
+ ereport(DEBUG1,
+ (errmsg_internal("sending backup \"%s\"", opt.label)));
+
/* Create a basic basebackup sink. */
sink = bbsink_copytblspc_new();
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 7a7eb3784e..f0062aa615 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -712,6 +712,10 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
LSN_FORMAT_ARGS(*startpoint));
set_ps_display(activitymsg);
}
+
+ ereport(DEBUG1,
+ (errmsg_internal("restarting at %X/%X",
+ LSN_FORMAT_ARGS(*startpoint;
}
/*
@@ -1000,6 +1004,10 @@ XLogWalRcvFlush(bool dying, TimeLineID tli)
set_ps_display(activitymsg);
}
+ ereport(DEBUG1,
+(errmsg_internal("streaming %X/%X",
+ LSN_FORMAT_ARGS(LogstreamResult.Write;
+
/* Also let the primary know that we made some progress */
if (!dying)
{
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 7950afb173..e04bab54c9 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2948,6 +2948,10 @@ retry:
LSN_FORMAT_ARGS(sentPtr));
set_ps_display(activitymsg);
}
+
+ ereport(DEBUG1,
+ (errmsg_internal("streaming %X/%X",
+ LSN_FORMAT_ARGS(sentPtr;
}
/*
--
2.25.1
Re: support for MERGE
On Sun, Nov 14, 2021 at 12:23 AM Álvaro Herrera wrote: > On 2021-Nov-13, Daniel Westermann wrote: > > /usr/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv -O2 > > -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -flto=thin > > -emit-llvm -c -o execMerge.bc execMerge.c > > execMerge.c:552:32: warning: if statement has empty body [-Wempty-body] > > RELKIND_PARTITIONED_TABLE); > > ^ > > execMerge.c:552:32: note: put the semicolon on a separate line to silence > > this warning > > Oh wow, this may be a pretty serious problem actually. I think it > represents a gap in testing. Thanks for reporting. Ah, thanks indeed. It seems that I fat-fingered that semicolon in. Though, it's not as serious as it would've been if I had instead fat-fingered a `&& false` into that condition and not the semicolon. ;) The only problem caused by the code block that follows the buggy if statement unconditionally executing is wasted cycles. Fortunately, there's no correctness issue, because rootRelInfo is the same as the input result relation in the cases where the latter is not partitioned and there'd be no map to convert the tuple, so the block is basically a no-op. I was afraid about the case where the input relation is a regular inheritance parent, though apparently we don't support MERGE in that case to begin with. -- Amit Langote EDB: http://www.enterprisedb.com
Emit a warning if the extension's GUC is set incorrectly
Hi hackers,
If wrong GUCs of auth_delay, pg_trgm, postgres_fdw and sepgsql are
described in postgresql.conf, a warning is not emitted unlike
pg_stat_statements, auto_explain and pg_prewarm.
So, I improved it by adding EmitWarningsOnPlaceholders.
An example output is shown below.
---
2021-11-14 18:18:16.486 JST [487067] WARNING: unrecognized
configuration parameter "auth_delay.xxx"
---
What do you think?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index 5820ac328d..d11dd1e416 100644
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -67,6 +67,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("auth_delay");
+
/* Install Hooks */
original_client_auth_hook = ClientAuthentication_hook;
ClientAuthentication_hook = auth_delay_checks;
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index fb38135f7a..0407c7dd64 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -100,6 +100,8 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("pg_trgm");
}
/*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 48c7417e6e..36555398ec 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -469,4 +469,6 @@ _PG_init(void)
NULL,
NULL,
NULL);
+
+ EmitWarningsOnPlaceholders("postgres_fdw");
}
diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index 19a3ffb7ff..44a09c35a7 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -455,6 +455,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("sepgsql");
+
/* Initialize userspace access vector cache */
sepgsql_avc_init();
Re: Commitfest 2021-11 Patch Triage - Part 2
> On 11/10/21 16:54, Andrey Borodin wrote: > >> Compression is crucial for highly available setups. Replication traffic is >> often billed. Or route has bandwidth limits. >> An entropy added by WAL headers makes CRIME attack against replication >> encryption impractical. > > I very much doubt WAL headers are a reliable protection against CRIME, > because the entropy of the headers is likely fairly constant. So if you > compress the WAL stream, the WAL headers may change but the compression > ratio should be pretty similar. At least that's my guess. I've thought more about it and I agree. To reliably protect against CRIME entropy of WAL headers must be comparable with the entropy of possibly injected data. If this would stand, probably, our WAL would need a really serious rework. Maybe just refuse to enable compression on SSL connection? If someone really needs both - they will just patch a server on their own. Or make a GUC "yes_i_kwow_what_crime_is_give_grant_read_on_my_data_to_spies". Best regards, Andrey Borodin.
Re: make update-po problem with USE_PGXS
On 01.11.21 04:56, wangsh.f...@fujitsu.com wrote:
I'm developing an extension, after modifying some source file(add some message
like gettext('x')) and execute
$ make USE_PGXS=1 update-po
make: Nothing to be done for 'update-po'.
I feel strange because I think *.po.new files should be created.
I don't think the NLS makefiles were ever adjusted to work with PGXS.
It's not surprising to me that some amount of adjustment appears to be
necessary to make the paths right.
