Re: Add LZ4 compression in pg_dump
On Fri, Mar 25, 2022 at 11:43:17PM +, gkokola...@pm.me wrote: > On Saturday, March 26th, 2022 at 12:13 AM, Rachel Heaton > wrote: >> Here is an updated patchset from Georgios, with minor assistance from myself. >> The comments above should be addressed, but please let us know if > > A small amendment to the above statement. This patchset does not include the > refactoring of compress_io suggested by Mr Paquier in the same thread, as it > is > missing documentation. An updated version will be sent to include those > changes > on the next commitfest. The refactoring using callbacks would make the code much cleaner IMO in the long term, with zstd waiting in the queue. Now, I see some pieces of the patch set that could be merged now without waiting for the development cycle of 16 to begin, as of 0001 to add more tests and 0002. I have a question about 0002, actually. What has led you to the conclusion that this code is dead and could be removed? -- Michael signature.asc Description: PGP signature
Re: Remove an unused function GetWalRcvWriteRecPtr
On Sat, Mar 26, 2022 at 10:51:15AM +0530, Bharath Rupireddy wrote: > The function GetWalRcvWriteRecPtr isn't being used anywhere, however > pg_atomic_read_u64(>writtenUpto); (reading writtenUpto without > spinlock) is being used directly in pg_stat_get_wal_receiver > walreceiver.c. We either make use of the function instead of > pg_atomic_read_u64(>writtenUpto); or remove it. Since there's > only one function using walrcv->writtenUpto right now, I prefer to > remove the function to save some LOC (13). > > Attaching patch. Thoughts? This could be used by some external module, no? -- Michael signature.asc Description: PGP signature
Re: pg_tablespace_location() failure with allow_in_place_tablespaces
On Thu, Mar 24, 2022 at 04:41:30PM +1300, Thomas Munro wrote: > As mentioned, I was unhappy with the lack of error checking for that > interface, and I've started a new thread about that, but then I > started wondering if we missed a trick here: get_dirent_type() contain > code that wants to return PGFILETYPE_LNK for reparse points. Clearly > it's not working, based on results reported in this thread. Is that > explained by your comment above, "junction points _are_ directories", > and we're testing the attribute flags in the wrong order here? > > if ((fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0) > d->ret.d_type = DT_DIR; > /* For reparse points dwReserved0 field will contain the ReparseTag */ > else if ((fd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0 && > (fd.dwReserved0 == IO_REPARSE_TAG_MOUNT_POINT)) > d->ret.d_type = DT_LNK; > else > d->ret.d_type = DT_REG; Ah, good point. I have not tested on Windows so I am not 100% sure, but indeed it would make sense to reverse both conditions if a junction point happens to be marked as both FILE_ATTRIBUTE_DIRECTORY and FILE_ATTRIBUTE_REPARSE_POINT when scanning a directory. Based on a read of the the upstream docs, I guess that this is the case: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/ca28ec38-f155-4768-81d6-4bfeb8586fc9 Note the "A file or directory that has an associated reparse point." for the description of FILE_ATTRIBUTE_REPARSE_POINT. -- Michael signature.asc Description: PGP signature
Remove an unused function GetWalRcvWriteRecPtr
Hi, The function GetWalRcvWriteRecPtr isn't being used anywhere, however pg_atomic_read_u64(>writtenUpto); (reading writtenUpto without spinlock) is being used directly in pg_stat_get_wal_receiver walreceiver.c. We either make use of the function instead of pg_atomic_read_u64(>writtenUpto); or remove it. Since there's only one function using walrcv->writtenUpto right now, I prefer to remove the function to save some LOC (13). Attaching patch. Thoughts? Regards, Bharath Rupireddy. v1-0001-Remove-an-unused-function-GetWalRcvWriteRecPtr.patch Description: Binary data
Re: pg_walinspect - a new extension to get raw WAL data and WAL stats
On Fri, Mar 25, 2022 at 8:37 PM RKN Sai Krishna
wrote:
>
> Hi Bharath,
>
> First look at the patch, bear with me if any of the following comments are
> repeated.
Thanks RKN, for playing around with the patches.
> 1. With pg_get_wal_record(lsn), say a WAL record start, end lsn range
> contains the specified LSN, wouldn't it be more meaningful to show the
> corresponding WAL record.
In general, all the functions will first look for a first valid WAL
record from the given input lsn/start lsn(XLogFindNextRecord) and then
give info of all the valid records including the first valid WAL
record until either the given end lsn or till end of WAL depending on
the function used.
> For example, upon providing '0/17335E7' as input, and I see get the WAL
> record ('0/1733618', '0/173409F') as output and not the one with start and
> end lsn as ('0/17335E0', '0/1733617').
If '0/17335E7' is an LSN containing a valid WAL record,
pg_get_wal_record gives the info of that, otherwise if there's any
next valid WAL record, it finds and gives that info. '0/17335E0' is
before '0/17335E7' the input lsn, so it doesn't show that record, but
the next valid record.
All the pg_walinspect functions don't look for the nearest valid WAL
record (could be previous to input lsn or next to input lsn), but they
look for the next valid WAL record. This is because the xlogreader
infra now has no API for backward iteration from a given LSN ( it has
XLogFindNextRecord and XLogReadRecord which scans the WAL in forward
direction). But, it's a good idea to have XLogFindPreviousRecord and
XLogReadPreviousRecord versions (as we have links for previous WAL
record in each WAL record) but that's a separate discussion.
> With pg_walfile_name(lsn), we can find the WAL segment file name that
> contains the specified LSN.
Yes.
> 2. I see the following output for pg_get_wal_record. Need to have a look at
> the spaces I suppose.
I believe this is something psql does for larger column outputs for
pretty-display. When used in a non-psql client, the column values are
returned properly. Nothing to do with the pg_walinspect patches here.
> 3. Should these functions be running in standby mode too? We do not allow WAL
> control functions to be executed during recovery right?
There are functions that can be executable during recovery
pg_last_wal_receive_lsn, pg_last_wal_replay_lsn. The pg_walinspect
functions are useful even in recovery and I don't see a strong reason
to not support them. Hence, I'm right now supporting them.
> 4. If the wal segment corresponding to the start lsn is removed, but there
> are WAL records which could be read in the specified input lsn range, would
> it be better to output the existing WAL records displaying a message that it
> is a partial list of WAL records and the WAL files corresponding to the rest
> are already removed, rather than erroring out saying "requested WAL segment
> has already been removed"?
"requested WAL segment %s has already been removed" is a common error
across the xlogreader infra (see wal_segment_open) and I don't want to
invent a new behaviour. And all the segment_open callbacks report an
error when they are not finding the WAL file that they are looking
for.
> 5. Following are very minor comments in the code
>
> Correct the function description by removing "return the LSN up to which the
> server has WAL" for IsFutureLSN
That's fine, because it actually returns curr_lsn via the function
param curr_lsn. However, I modified the comment a bit.
> In GetXLogRecordInfo, good to have pfree in place for rec_desc, rec_blk_ref,
> data
No, we are just returning pointer to the string, not deep copying, see
CStringGetTextDatum. All the functions get executed within a
function's memory context and after handing off the results to the
client that gets deleted, deallocating all the memory.
> In GetXLogRecordInfo, can avoid calling XLogRecGetInfo(record) multiple times
> by capturing in a variable
XLogRecGetInfo is not a function, it's a macro, so that's fine.
#define XLogRecGetInfo(decoder) ((decoder)->record->header.xl_info)
> In GetWALDetailsGuts, setting end_lsn could be done in single if else and
> similarly we can club the if statements verifying if the start lsn is a
> future lsn.
The existing if conditions are:
if (IsFutureLSN(start_lsn, _lsn))
if (!till_end_of_wal && end_lsn >= curr_lsn)
if (till_end_of_wal)
if (start_lsn >= end_lsn)
I clubbed them like this:
if (!till_end_of_wal)
if (IsFutureLSN(start_lsn, _lsn))
if (!till_end_of_wal && end_lsn >= curr_lsn)
else if (till_end_of_wal)
Other if conditions are serving different purposes, so I'm leaving them as-is.
Attaching v16 patch-set, only change in v16-0002-pg_walinspect.patch,
others remain the same.
Regards,
Bharath Rupireddy.
v16-0001-Refactor-pg_waldump-code.patch
Description: Binary data
v16-0002-pg_walinspect.patch
Description: Binary data
v16-0003-pg_walinspect-tests.patch
RE: Column Filtering in Logical Replication
Hello, The 'prattrs' column has been added to the pg_publication_rel catalog, but the current commit to catalog.sgml seems to have added it to pg_publication_namespace. The attached patch fixes this. Regards, Noriyoshi Shinoda -Original Message- From: Tomas Vondra Sent: Saturday, March 26, 2022 9:35 AM To: Amit Kapila Cc: Peter Eisentraut ; houzj.f...@fujitsu.com; Alvaro Herrera ; Justin Pryzby ; Rahila Syed ; Peter Smith ; pgsql-hackers ; shiy.f...@fujitsu.com Subject: Re: Column Filtering in Logical Replication On 3/26/22 01:18, Tomas Vondra wrote: > > ... > > I went over the patch again, polished the commit message a bit, and > pushed. May the buildfarm be merciful! > There's a couple failures immediately after the push, which caused me a minor heart attack. But it seems all of those are strange failures related to configure (which the patch did not touch at all), on animals managed by Andres. And a couple animals succeeded since then. So I guess the animals were reconfigured, or something ... regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company prattrs_column_fix_v1.diff Description: prattrs_column_fix_v1.diff
Re: pgsql: Add 'basebackup_to_shell' contrib module.
On Fri, Mar 25, 2022 at 02:27:07PM -0700, Andres Freund wrote: > On 2022-03-25 13:52:11 -0400, Robert Haas wrote: > > On Fri, Mar 25, 2022 at 12:36 PM Andres Freund wrote: > > > Create a CF entry for it, or enable CI on a github repo? > > > > I created a CF entry for it. Then I had to try to Google around to > > find the URL from the cfbot, because it's not even linked from > > commitfest.postgresql.org for some reason. #blamemagnus I see it here (and in cfbot), although I'm not sure how you created a new patch for the active CF, and not for the next CF. https://commitfest.postgresql.org/37/ > > I don't think that the Windows CI is running the TAP tests for > > contrib. At least, I can't find any indication of it in the output. So > > it doesn't really help to assess how portable this test is, unless I'm > > missing something. > > Yea. It's really unfortunate how vcregress.pl makes it hard to run all > tests. And we're kind of stuck finding a way forward. It's easy enough to work > around for individual tests by just adding them to the test file (like Thomas > did nearby), but clearly that doesn't scale. Andrew wasn't happy with > additional vcregress commands. The fact that vcregress doesn't run tests in > parallel makes things take forever. And so it goes on. I have a patch to add alltaptests target to vcregress. But I don't recall hearing any objection to new targets until now. https://github.com/justinpryzby/postgres/runs/5174877506
Re: ubsan
Hi,
On 2022-03-23 15:55:28 -0700, Andres Freund wrote:
> Originally I'd planned to mix them into existing members, but I think it'd be
> better to have dedicated ones. Applied for a few new buildfarm names for:
> {gcc,clang}-{-fsanitize=undefined,-fsanitize=address}.
They're now enabled...
tamandua: gcc, -fsanitize=undefined,alignment
kestrel: clang, -fsanitize=undefined,alignment
grassquit: gcc, -fsanitize=address
olingo: clang, -fsanitize=address
The first three have started reporting in, the last is starting its first run
now.
Greetings,
Andres Freund
Re: Column Filtering in Logical Replication
On 3/26/22 01:18, Tomas Vondra wrote: > > ... > > I went over the patch again, polished the commit message a bit, and > pushed. May the buildfarm be merciful! > There's a couple failures immediately after the push, which caused me a minor heart attack. But it seems all of those are strange failures related to configure (which the patch did not touch at all), on animals managed by Andres. And a couple animals succeeded since then. So I guess the animals were reconfigured, or something ... regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Column Filtering in Logical Replication
On 3/21/22 15:12, Amit Kapila wrote:
> On Sat, Mar 19, 2022 at 11:11 PM Tomas Vondra
> wrote:
>>
>> On 3/19/22 18:11, Tomas Vondra wrote:
>>> Fix a compiler warning reported by cfbot.
>>
>> Apologies, I failed to actually commit the fix. So here we go again.
>>
>
> Few comments:
> ===
> 1.
> +/*
> + * Gets a list of OIDs of all partial-column publications of the given
> + * relation, that is, those that specify a column list.
> + */
> +List *
> +GetRelationColumnPartialPublications(Oid relid)
> {
> ...
> }
>
> ...
> +/*
> + * For a relation in a publication that is known to have a non-null column
> + * list, return the list of attribute numbers that are in it.
> + */
> +List *
> +GetRelationColumnListInPublication(Oid relid, Oid pubid)
> {
> ...
> }
>
> Both these functions are not required now. So, we can remove them.
>
Good catch, removed.
> 2.
> @@ -464,11 +478,11 @@ logicalrep_write_update(StringInfo out,
> TransactionId xid, Relation rel,
> pq_sendbyte(out, 'O'); /* old tuple follows */
> else
> pq_sendbyte(out, 'K'); /* old key follows */
> - logicalrep_write_tuple(out, rel, oldslot, binary);
> + logicalrep_write_tuple(out, rel, oldslot, binary, columns);
> }
>
> As mentioned previously, here, we should pass NULL similar to
> logicalrep_write_delete as we don't need to use column list for old
> tuples.
>
Fixed.
> 3.
> + * XXX The name is a bit misleading, because we don't really transform
> + * anything here - we merely check the column list is compatible with the
> + * definition of the publication (with publish_via_partition_root=false)
> + * we only allow column lists on the leaf relations. So maybe rename it?
> + */
> +static void
> +TransformPubColumnList(List *tables, const char *queryString,
> +bool pubviaroot)
>
> The second parameter is not used in this function. As noted in the
> comments, I also think it is better to rename this. How about
> ValidatePubColumnList?
>
> 4.
> @@ -821,6 +942,9 @@ fetch_remote_table_info(char *nspname, char *relname,
> *
> * 3) one of the subscribed publications is declared as ALL TABLES IN
> * SCHEMA that includes this relation
> + *
> + * XXX Does this actually handle puballtables and schema publications
> + * correctly?
> */
> if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 15)
>
> Why is this comment added in the row filter code? Now, both row filter
> and column list are fetched in the same way, so not sure what exactly
> this comment is referring to.
>
I added that comment as a note to myself while learning about how the
code works, forgot to remove that.
> 5.
> +/* qsort comparator for attnums */
> +static int
> +compare_int16(const void *a, const void *b)
> +{
> + int av = *(const int16 *) a;
> + int bv = *(const int16 *) b;
> +
> + /* this can't overflow if int is wider than int16 */
> + return (av - bv);
> +}
>
> The exact same code exists in statscmds.c. Do we need a second copy of the
> same?
>
Yeah, I thought about moving it to some common header, but I think it's
not really worth it at this point.
> 6.
> static void pgoutput_row_filter_init(PGOutputData *data,
> List *publications,
> RelationSyncEntry *entry);
> +
> static bool pgoutput_row_filter_exec_expr(ExprState *state,
>
> Spurious line addition.
>
Fixed.
> 7. The tests in 030_column_list.pl take a long time as compared to all
> other similar individual tests in the subscription folder. I haven't
> checked whether there is any need to reduce some tests but it seems
> worth checking.
>
On my machine, 'make check' in src/test/subscription takes ~150 seconds
(with asserts and -O0), and the new script takes ~14 seconds, while most
other tests have 3-6 seconds.
AFAICS that's simply due to the number of tests in the script, and I
don't think there are any unnecessary ones. I was actually adding them
in response to issues reported during development, or to test various
important cases. So I don't think we can remove some of them easily :-(
And it's not like the tests are using massive amounts of data either.
We could split the test, but that obviously won't reduce the duration,
of course.
So I decided to keep the test as is, for now, and maybe we can try
reducing the test after a couple buildfarm runs.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Column Filtering in Logical Replication
On 3/25/22 04:10, Amit Kapila wrote: > On Fri, Mar 25, 2022 at 5:44 AM Tomas Vondra > wrote: >> >> Attached is a patch, rebased on top of the sequence decoding stuff I >> pushed earlier today, also including the comments rewording, and >> renaming the "transform" function. >> >> I'll go over it again and get it pushed soon, unless someone objects. >> > > You haven't addressed the comments given by me earlier this week. See > https://www.postgresql.org/message-id/CAA4eK1LY_JGL7LvdT64ujEiEAVaADuhdej1QNnwxvO_-KPzeEg%40mail.gmail.com. > Thanks for noticing that! Thunderbird did not include that message into the patch thread for some reason, so I did not notice that! > * > + * XXX The name is a bit misleading, because we don't really transform > + * anything here - we merely check the column list is compatible with the > + * definition of the publication (with publish_via_partition_root=false) > + * we only allow column lists on the leaf relations. So maybe rename it? > + */ > +static void > +CheckPubRelationColumnList(List *tables, const char *queryString, > +bool pubviaroot) > > After changing this function name, the comment above is not required. > Thanks, comment updated. I went over the patch again, polished the commit message a bit, and pushed. May the buildfarm be merciful! -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
On Fri, Mar 25, 2022 at 10:09:33PM +0100, Daniel Gustafsson wrote:
> Agreed. In this case it seems that adding --exclude-extension would make
> sense
> to keep conistency. I took a quick stab at doing so with the attached while
> we're here.
src/test/modules/test_pg_dump would be the best place for the addition
of a couple of tests with this new switch. Better to check as well
what happens when a command collides with --extension and
--exclude-extension.
printf(_(" -e, --extension=PATTERN dump the specified extension(s)
only\n"));
+ printf(_(" --exclude-extension=PATTERN do NOT dump the specified
extension(s)\n"));
Shouldn't this be listed closer to --exclude-table-data in the --help
output?
--
Michael
signature.asc
Description: PGP signature
Re: Add LZ4 compression in pg_dump
--- Original Message --- On Saturday, March 26th, 2022 at 12:13 AM, Rachel Heaton wrote: > On Fri, Mar 25, 2022 at 6:22 AM Justin Pryzby pry...@telsasoft.com wrote: > > > On Fri, Mar 25, 2022 at 01:20:47AM -0400, Greg Stark wrote: > > > > > It seems development on this has stalled. If there's no further work > > > happening I guess I'll mark the patch returned with feedback. Feel > > > free to resubmit it to the next CF when there's progress. We had some progress yet we didn't want to distract the list with too many emails. Of course, it seemed stalled to the outside observer, yet I simply wanted to set the record straight and say that we are actively working on it. > > > > Since it's a reasonably large patch (and one that I had myself started > > before) > > and it's only been 20some days since (minor) review comments, and since the > > focus right now is on committing features, and not reviewing new patches, > > and > > this patch is new one month ago, and its 0002 not intended for pg15, > > therefor > > I'm moving it to the next CF, where I hope to work with its authors to > > progress > > it. Thank you. It is much appreciated. We will sent updates when the next commitfest starts in July as to not distract from the 15 work. Then, we can take it from there. > > Hi Folks, > > Here is an updated patchset from Georgios, with minor assistance from myself. > The comments above should be addressed, but please let us know if A small amendment to the above statement. This patchset does not include the refactoring of compress_io suggested by Mr Paquier in the same thread, as it is missing documentation. An updated version will be sent to include those changes on the next commitfest. > there are other things to go over. A functional change in this > patchset is when `--compress=none` is passed to pg_dump, it will not > compress for directory type (previously, it would use gzip if > present). The previous default behavior is retained. > > - Rachel
Re: [PATCH] Enable SSL library detection via PQsslAttribute
On Fri, 2022-03-25 at 18:00 -0400, Tom Lane wrote: > Jacob Champion writes: > > Do I need to merge my tiny test program into the libpq_pipeline tests? > > Doesn't seem worth the trouble to me, notably because you'd > then have to cope with non-SSL builds too. Fine by me. v5 moves the docs out of the Note, as requested by Daniel. Thanks, --Jacob commit 585bf50bdd9ffc1b7c07b65e500a19bf50fbddff Author: Jacob Champion Date: Fri Mar 25 15:36:09 2022 -0700 squash! Enable SSL library detection via PQsslAttribute() Move new docs out of a Note into the main body. diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 82f3092715..aa400d1b7c 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -2538,17 +2538,6 @@ const char *PQsslAttribute(const PGconn *conn, const char *attribute_name); Name of the SSL implementation in use. (Currently, only "OpenSSL" is implemented) - - - As a special case, the library attribute may be - queried without an existing connection by passing NULL as the - conn argument. The historical behavior was to - return NULL for any attribute when a NULL conn - was provided; client programs needing to differentiate between the - newer and older implementations may check the - LIBPQ_HAS_SSL_LIBRARY_DETECTION feature macro. - - @@ -2592,6 +2581,16 @@ const char *PQsslAttribute(const PGconn *conn, const char *attribute_name); + + + As a special case, the library attribute may be + queried without an existing connection by passing NULL as the + conn argument. The historical behavior was to return + NULL for any attribute when a NULL conn was provided; + client programs needing to differentiate between the newer and older + implementations may check the + LIBPQ_HAS_SSL_LIBRARY_DETECTION feature macro. + From 6ea7a272a4cf316d4bf94da07c1d3538fcaa333e Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 29 Nov 2021 14:36:38 -0800 Subject: [PATCH v5] Enable SSL library detection via PQsslAttribute() Currently, libpq client code must have a connection handle before it can query the "library" SSL attribute. This poses problems if the client needs to know what SSL library is in use before constructing a connection string. (For example, with the NSS proposal, a client would have to decide whether to use the "ssldatabase" connection setting rather than "sslcert" et al.) Allow PQsslAttribute(NULL, "library") to return the library in use -- currently, just "OpenSSL" or NULL. The new behavior is announced with the LIBPQ_HAS_SSL_LIBRARY_DETECTION feature macro, allowing clients to differentiate between a libpq that was compiled without SSL support and a libpq that's just too old to tell. --- doc/src/sgml/libpq.sgml | 10 +++ src/interfaces/libpq/Makefile| 1 + src/interfaces/libpq/fe-secure-openssl.c | 6 ++-- src/interfaces/libpq/libpq-fe.h | 2 ++ src/interfaces/libpq/t/002_api.pl| 23 +++ src/interfaces/libpq/test/.gitignore | 1 + src/interfaces/libpq/test/Makefile | 2 +- src/interfaces/libpq/test/testclient.c | 37 8 files changed, 78 insertions(+), 4 deletions(-) create mode 100644 src/interfaces/libpq/t/002_api.pl create mode 100644 src/interfaces/libpq/test/testclient.c diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 3998b1781b..aa400d1b7c 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -2581,6 +2581,16 @@ const char *PQsslAttribute(const PGconn *conn, const char *attribute_name); + + + As a special case, the library attribute may be + queried without an existing connection by passing NULL as the + conn argument. The historical behavior was to return + NULL for any attribute when a NULL conn was provided; + client programs needing to differentiate between the newer and older + implementations may check the + LIBPQ_HAS_SSL_LIBRARY_DETECTION feature macro. + diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile index 3c53393fa4..89bf5e0126 100644 --- a/src/interfaces/libpq/Makefile +++ b/src/interfaces/libpq/Makefile @@ -13,6 +13,7 @@ subdir = src/interfaces/libpq top_builddir = ../../.. include $(top_builddir)/src/Makefile.global +export with_ssl PGFILEDESC = "PostgreSQL Access Library" diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index d81218a4cc..d3bf57b850 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -1631,14 +1631,14 @@ PQsslAttributeNames(PGconn *conn) const char
Re: make MaxBackends available in _PG_init
Hi, On 2022-03-25 14:35:42 +0800, Julien Rouhaud wrote: > On Fri, Mar 25, 2022 at 02:08:09PM +0900, Michael Paquier wrote: > > On Fri, Mar 25, 2022 at 11:11:46AM +0800, Julien Rouhaud wrote: > > > As an example, here's a POC for a new shmem_request_hook hook after > > > _PG_init(). > > > With it I could easily fix pg_wait_sampling shmem allocation (and checked > > > that > > > it's indeed requesting the correct size). > > > > Are you sure that the end of a release cycle is the good moment to > > begin designing new hooks? Anything added is something we are going > > to need supporting moving forward. My brain is telling me that we > > ought to revisit the business with GetMaxBackends() properly instead, > > and perhaps revert that. > > I agree, and as I mentioned in my original email I don't think that the > committed patch is actually adding something on which we can really build on. > So I'm also in favor of reverting, as it seems like be a better option in the > long run to have a clean and broader solution. I don't really understand. The issue that started this thread was bugs in extensions due to accessing MaxBackends before it is initialized - which the patch prevents. The stuff that you're complaining about / designing here doesn't seem related to that. I like the idea of the hooks etc, but I fail to see why we "ought to revisit the business with GetMaxBackends()"? Greetings, Andres Freund
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
"David G. Johnston" writes: > If we want to choose the other position I would just go with > "--[no]-system-objects" options to toggle whether pattern matching grabs > them by default (defaulting to no) and if someone wants to enable them for > only specific object types they can --system-objects and then > --exclude-type='pg_catalog' any that shouldn't be enabled. Yeah, I could live with that. Per-object-type control doesn't seem necessary. regards, tom lane
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
On Fri, Mar 25, 2022 at 2:55 PM Tom Lane wrote: > Daniel Gustafsson writes: > >> On 25 Mar 2022, at 19:37, Tom Lane wrote: > >> I'd vote for changing the behavior of --table rather than trying to > >> be bug-compatible with this decision. > > > Agreed. Question is what to do for "-t pg_class", should we still forbid > > dumping system catalogs when they are pattern matched without wildcard > or is > > should that be ok? And should this depend on if "-n pg_catalog" is used? > > I don't think there's anything really wrong with just "we won't dump > system objects, full stop"; I don't see much use-case for doing that > except maybe debugging, and even that is a pretty thin argument. > +1 We could bug-fix in a compromise if we felt compelled by a user complaint but I don't foresee any compelling ones for this. The catalogs are implementation details that should never have been exposed in this manner in the first place. If we want to choose the other position I would just go with "--[no]-system-objects" options to toggle whether pattern matching grabs them by default (defaulting to no) and if someone wants to enable them for only specific object types they can --system-objects and then --exclude-type='pg_catalog' any that shouldn't be enabled. The documentation already says that the include options ignore -n/-N so the solution that breaks this rule seems less appealing at a cursory glance. David J.
Re: [PATCH] Enable SSL library detection via PQsslAttribute
Jacob Champion writes: > Do I need to merge my tiny test program into the libpq_pipeline tests? Doesn't seem worth the trouble to me, notably because you'd then have to cope with non-SSL builds too. regards, tom lane
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
Daniel Gustafsson writes: >> On 25 Mar 2022, at 19:37, Tom Lane wrote: >> I'd vote for changing the behavior of --table rather than trying to >> be bug-compatible with this decision. > Agreed. Question is what to do for "-t pg_class", should we still forbid > dumping system catalogs when they are pattern matched without wildcard or is > should that be ok? And should this depend on if "-n pg_catalog" is used? I don't think there's anything really wrong with just "we won't dump system objects, full stop"; I don't see much use-case for doing that except maybe debugging, and even that is a pretty thin argument. However, a possible compromise is to say that we act as though --exclude-schema=pg_catalog is specified unless you explicitly override that with "--schema=pg_catalog". (And the same for information_schema, I suppose.) This might be a bit hacky to implement :-( regards, tom lane
Re: pgsql: Add 'basebackup_to_shell' contrib module.
On Sat, Mar 26, 2022 at 9:55 AM Thomas Munro wrote: > https://api.cirrus-ci.com/v1/artifact/task/5637156969381888/log/contrib/basebackup_to_shell/tmp_check/log/regress_log_001_basic This line doesn't look too healthy: pg_basebackup: error: backup failed: ERROR: shell command "type con > "C:cirruscontrib asebackup_to_shell mp_check mp_test_tch3\base.tar"" failed I guess it's an escaping problem around \ characters.
Re: Deparsing rewritten query
Julien Rouhaud writes: > I'm attaching the correct patch this time, sorry about that. While I'm okay with this in principle, as it stands it fails headerscheck/cpluspluscheck: $ src/tools/pginclude/headerscheck In file included from /tmp/headerscheck.Oi8jj3/test.c:2: ./src/include/utils/ruleutils.h:42:35: error: unknown type name 'StringInfo'; did you mean 'String'? void get_query_def(Query *query, StringInfo buf, List *parentnamespace, ^~ String ./src/include/utils/ruleutils.h:43:9: error: unknown type name 'TupleDesc' TupleDesc resultDesc, ^ We could of course add the necessary #include's to ruleutils.h, but considering that we seem to have been at some pains to minimize its #include footprint, I'm not really happy with that approach. I'm inclined to think that maybe a wrapper function with a slightly simplified interface would be a better way to go. The deparsed string could just be returned as a palloc'd "char *", unless you have some reason to need it to be in a StringInfo. I wonder which of the other parameters really need to be exposed, too. Several of them seem to be more internal to ruleutils.c than something that outside callers would care to manipulate. For instance, since struct deparse_namespace isn't exposed, there really isn't any way to pass anything except NIL for parentnamespace. The bigger picture here is that get_query_def's API has changed over time internally to ruleutils.c, and I kind of expect that that might continue in future, so having a wrapper function with a more stable API could be a good thing. regards, tom lane
Re: [PATCH] Enable SSL library detection via PQsslAttribute
> On 25 Mar 2022, at 22:25, Jacob Champion wrote: > On Fri, 2022-03-25 at 15:32 -0400, Robert Haas wrote: >> This seems totally reasonable. However, I think it should update the >> documentation somehow. > > Done in v4. I would prefer to not introduce a for this, I think adding it as a under PQsslAttribute is better given the rest of the libpq API documentation. The proposed text reads fine to me. -- Daniel Gustafsson https://vmware.com/
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
> On 25 Mar 2022, at 19:37, Tom Lane wrote: > I'd vote for changing the behavior of --table rather than trying to > be bug-compatible with this decision. Agreed. Question is what to do for "-t pg_class", should we still forbid dumping system catalogs when they are pattern matched without wildcard or is should that be ok? And should this depend on if "-n pg_catalog" is used? -- Daniel Gustafsson https://vmware.com/
Re: pgsql: Add 'basebackup_to_shell' contrib module.
Hi, On 2022-03-25 13:52:11 -0400, Robert Haas wrote: > On Fri, Mar 25, 2022 at 12:36 PM Andres Freund wrote: > > Create a CF entry for it, or enable CI on a github repo? > > I created a CF entry for it. Then I had to try to Google around to > find the URL from the cfbot, because it's not even linked from > commitfest.postgresql.org for some reason. #blamemagnus Yea, we really need to improve on that. I think Thomas has some hope of improving things after the release... > I don't think that the Windows CI is running the TAP tests for > contrib. At least, I can't find any indication of it in the output. So > it doesn't really help to assess how portable this test is, unless I'm > missing something. Yea. It's really unfortunate how vcregress.pl makes it hard to run all tests. And we're kind of stuck finding a way forward. It's easy enough to work around for individual tests by just adding them to the test file (like Thomas did nearby), but clearly that doesn't scale. Andrew wasn't happy with additional vcregress commands. The fact that vcregress doesn't run tests in parallel makes things take forever. And so it goes on. > I looked through the Linux output. It looks to me like that does run > the TAP tests for contrib. Unfortunately, the output is not in order > and is also not labelled, so it's hard to tell what output goes with > what contrib module. I named my test 001_basic.pl, but there are 12 of > those already. I see that 13 copies of 001_basic.pl seem to have > passed CI on Linux, so I guess the test ran and passed there. It seems > like it would be an awfully good idea to mention the subdirectory name > before each dump of output. Yea, the current output is *awful*. FWIW, the way it's hard to run tests the same way across platforms, the crappy output etc was one of the motivations behind the meson effort. If you just compare the output from both *nix and windows runs today with the meson output, it's imo night and day: https://cirrus-ci.com/task/5869668815601664?logs=check_world#L67 That's a recent run where I'd not properly mirrored 7c51b7f7cc0, leading to a failure on windows. Though it'd be more intersting to see a run with a failure. If one wants one can also see the test output of individual tests (it's always logged to a file). But I typically find that not useful for a 'general' test run, too much output. In that case there's a nice list of failed tests at the end: Summary of Failures: 144/219 postgresql:tap+vacuumlo / vacuumlo/t/001_basic.pl ERROR 0.48s (exit status 255 or signal 127 SIGinvalid) Ok: 218 Expected Fail: 0 Fail: 1 Unexpected Pass:0 Skipped:0 Timeout:0 Greetings, Andres Freund
Re: [PATCH] Enable SSL library detection via PQsslAttribute
On Fri, 2022-03-25 at 15:32 -0400, Robert Haas wrote:
> On Mon, Feb 28, 2022 at 3:21 PM Jacob Champion wrote:
> > v3 rebases over Andres' changes and actually adds the Perl driver that
> > I missed the git-add for.
>
> This seems totally reasonable. However, I think it should update the
> documentation somehow.
Done in v4.
Do I need to merge my tiny test program into the libpq_pipeline tests?
I'm not sure what the roadmap is for those.
Thanks!
--Jacob
commit 53fca988682c80a99bbb19eeb3d7959533fc3b83
Author: Jacob Champion
Date: Fri Mar 25 14:16:47 2022 -0700
squash! Enable SSL library detection via PQsslAttribute()
Add docs.
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 3998b1781b..82f3092715 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2538,6 +2538,17 @@ const char *PQsslAttribute(const PGconn *conn, const
char *attribute_name);
Name of the SSL implementation in use. (Currently, only
"OpenSSL" is implemented)
+
+
+ As a special case, the library attribute may be
+ queried without an existing connection by passing NULL as the
+ conn argument. The historical behavior was to
+ return NULL for any attribute when a NULL conn
+ was provided; client programs needing to differentiate between the
+ newer and older implementations may check the
+ LIBPQ_HAS_SSL_LIBRARY_DETECTION feature macro.
+
+
From 1ec54121dc0eeae7e48b9e38b829980e6a11e31c Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Mon, 29 Nov 2021 14:36:38 -0800
Subject: [PATCH v4] Enable SSL library detection via PQsslAttribute()
Currently, libpq client code must have a connection handle before it can
query the "library" SSL attribute. This poses problems if the client
needs to know what SSL library is in use before constructing a
connection string. (For example, with the NSS proposal, a client would
have to decide whether to use the "ssldatabase" connection setting
rather than "sslcert" et al.)
Allow PQsslAttribute(NULL, "library") to return the library in use --
currently, just "OpenSSL" or NULL. The new behavior is announced with
the LIBPQ_HAS_SSL_LIBRARY_DETECTION feature macro, allowing clients to
differentiate between a libpq that was compiled without SSL support and
a libpq that's just too old to tell.
---
doc/src/sgml/libpq.sgml | 11 +++
src/interfaces/libpq/Makefile| 1 +
src/interfaces/libpq/fe-secure-openssl.c | 6 ++--
src/interfaces/libpq/libpq-fe.h | 2 ++
src/interfaces/libpq/t/002_api.pl| 23 +++
src/interfaces/libpq/test/.gitignore | 1 +
src/interfaces/libpq/test/Makefile | 2 +-
src/interfaces/libpq/test/testclient.c | 37
8 files changed, 79 insertions(+), 4 deletions(-)
create mode 100644 src/interfaces/libpq/t/002_api.pl
create mode 100644 src/interfaces/libpq/test/testclient.c
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 3998b1781b..82f3092715 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2538,6 +2538,17 @@ const char *PQsslAttribute(const PGconn *conn, const char *attribute_name);
Name of the SSL implementation in use. (Currently, only
"OpenSSL" is implemented)
+
+
+ As a special case, the library attribute may be
+ queried without an existing connection by passing NULL as the
+ conn argument. The historical behavior was to
+ return NULL for any attribute when a NULL conn
+ was provided; client programs needing to differentiate between the
+ newer and older implementations may check the
+ LIBPQ_HAS_SSL_LIBRARY_DETECTION feature macro.
+
+
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index 3c53393fa4..89bf5e0126 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -13,6 +13,7 @@ subdir = src/interfaces/libpq
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+export with_ssl
PGFILEDESC = "PostgreSQL Access Library"
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index d81218a4cc..d3bf57b850 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1631,14 +1631,14 @@ PQsslAttributeNames(PGconn *conn)
const char *
PQsslAttribute(PGconn *conn, const char *attribute_name)
{
+ if (strcmp(attribute_name, "library") == 0)
+ return "OpenSSL";
+
if (!conn)
return NULL;
if (conn->ssl == NULL)
return NULL;
- if (strcmp(attribute_name, "library") == 0)
- return "OpenSSL";
-
if (strcmp(attribute_name, "key_bits") == 0)
Re: SQL/JSON: JSON_TABLE
On Fri, Mar 25, 2022 at 1:30 PM Andrew Dunstan wrote: > > On 3/22/22 10:55, Daniel Gustafsson wrote: > >> On 22 Mar 2022, at 16:31, Andrew Dunstan wrote: > >> I'm planning on pushing the functions patch set this week and json-table > >> next week. > > My comments from 30827b3c-edf6-4d41-bbf1-298181874...@yesql.se are yet > to be > > addressed (or at all responded to) in this patchset. I'll paste the > ones which > > still apply to make it easier: > > > > > I think I have fixed all those. See attached. I haven't prepared a new > patch set for SQL/JSON functions because there's just one typo to fix, > but I won't forget it. Please let me know if there's anything else you see. > > > At this stage I think I have finished with the actual code, and I'm > concentrating on improving the docs a bit. > > > cheers > > > andrew > > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com Hi, w.r.t. 0001-SQL-JSON-functions-without-sql_json-GUC-v59.patch : + Datum *innermost_caseval = state->innermost_caseval; + bool *innermost_isnull = state->innermost_casenull; + + state->innermost_caseval = resv; + state->innermost_casenull = resnull; + + ExecInitExprRec(jve->formatted_expr, state, resv, resnull); + + state->innermost_caseval = innermost_caseval; + state->innermost_casenull = innermost_isnull; Code similar to the above block appears at least twice. Maybe extract into a helper func to reuse code. + * Evaluate a JSON path variable caching computed value. + */ +int +EvalJsonPathVar(void *cxt, char *varName, int varNameLen, Please add description for return value in the comment. + if (formatted && IsA(formatted, Const)) + return formatted; If formatted is NULL, it cannot be Const. So the if can be simplified: + if (IsA(formatted, Const)) For transformJsonConstructorOutput(), it seems the variable have_json is not used - you can drop the variable. + * Coerce a expression in JSON DEFAULT behavior to the target output type. a expression -> an expression Cheers
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
> On 25 Mar 2022, at 01:40, Tom Lane wrote: > > "David G. Johnston" writes: >> The extension object type does not seem to have gotten the >> --exclude-extension capability that it would need to conform to the general >> design exemplified by --table and hopefully extended out to the routine >> object types. > > We're not going to instantly build out every feature that would be > suggested by a roadmap. Agreed. In this case it seems that adding --exclude-extension would make sense to keep conistency. I took a quick stab at doing so with the attached while we're here. -- Daniel Gustafsson https://vmware.com/ 0001-First-WIP-stab-at-exclude-extension-for-pg_dump.patch Description: Binary data
Re: Document atthasmissing default optimization avoids verification table scan
On Fri, Mar 25, 2022 at 1:40 PM Robert Haas wrote: > On Tue, Jan 25, 2022 at 8:49 AM James Coleman wrote: > > Here's a version that looks like that. I'm not convinced it's an > > improvement over the previous version: again, I expect more advanced > > users to already understand this concept, and I think moving it to the > > ALTER TABLE page could very well have the effect of burying i(amidst > > the ton of detail on the ALTER TABLE page) concept that would be > > useful to learn early on in a tutorial like the DDL page. But if > > people really think this is an improvement, then I can acquiesce. > > I vote for rejecting both of these patches. > > 0001 adds the following sentence to the documentation: "A NOT > NULL constraint may be added to the new column in the same > statement without requiring scanning the table to verify the > constraint." My first reaction when I read this sentence was that it > was warning the user about the absence of a hazard that no one would > expect in the first place. I agree. The wording that would make one even consider this has yet to have been introduced at this point in the documentation. > 0002 moves some advice about adding columns with defaults from one > part of the documentation to another. Maybe that's a good idea, and > maybe it isn't, but it also rewords the advice, and in my opinion, the > new wording is less clear and specific than the existing wording. In the passing time I've had to directly reference the DDL chapter (which is a mix of reference material and tutorial) on numerous items so my desire to move the commentary away from here is less, but still I feel that the command reference page is the correct place for this kind of detail. If we took away too much info and made things less clear let's address that. It can't be that much, we are talking about basically a paragraph of text here. > It > also changes a sentence that mentions volatile defaults to give a > specific example of a volatile function -- clock_timestamp -- probably > because where the documentation was before that function was > mentioned. However, that sentence seems clear enough as it is and does not really need an example. > Nope, the usage and context in the patch is basically the same as the existing usage and context. David J.
Re: SSL/TLS instead of SSL in docs
> On 25 Mar 2022, at 20:58, Robert Haas wrote: > > On Wed, Sep 15, 2021 at 8:47 AM Daniel Gustafsson wrote: >> Since the approach taken wasn't to anyones liking, attached is a v4 (partly >> extracted from the previous patch) which only adds notes that SSL is used >> interchangeably with TLS in our documentation and configuration. > > I have actually been wondering why we have been insisting on calling > it SSL when it clearly is not. SSL has become the de facto term for a network channel encryption regardless of actual protocol used. Few use TLS, with most SSL/TLS is > However, if we're not ready/willing to make a bigger change, then doing as you > have proposed here seems fine to me. Thanks for review! Trying out again just now the patch still applies (with some offsets) and builds. -- Daniel Gustafsson https://vmware.com/
Re: Document atthasmissing default optimization avoids verification table scan
Robert Haas writes: > I vote for rejecting both of these patches. I see what James is on about here, but I agree that these specific changes don't help much. What would actually be desirable IMO is a separate section somewhere explaining the performance characteristics of ALTER TABLE. (We've also kicked around the idea of EXPLAIN for ALTER TABLE, but that's a lot more work.) This could coalesce the parenthetical remarks that exist in ddl.sgml as well as alter_table.sgml into something a bit more unified and perhaps easier to follow. In particular, it should start by defining what we mean by "table rewrite" and "table scan". I don't recall at the moment whether we define those in multiple places or not at all, but as things stand any such discussion would be pretty fragmented. regards, tom lane
Re: pgsql: Add 'basebackup_to_shell' contrib module.
On Sat, Mar 26, 2022 at 6:52 AM Robert Haas wrote:
> I don't think that the Windows CI is running the TAP tests for
> contrib. At least, I can't find any indication of it in the output. So
> it doesn't really help to assess how portable this test is, unless I'm
> missing something.
Yeah :-( vcregress.pl doesn't yet have an easy way to run around and
find contrib modules with tap tests and run them, for the CI script to
call. (I think there was a patch somewhere? I've been bitten by the
lack of this recently...)
In case it's helpful, here's how to run a specific contrib module's
TAP test by explicitly adding it. That'll run once I post this email,
but I already ran in it my own github account and it looks like this:
https://cirrus-ci.com/task/5637156969381888
https://api.cirrus-ci.com/v1/artifact/task/5637156969381888/log/contrib/basebackup_to_shell/tmp_check/log/regress_log_001_basic
https://api.cirrus-ci.com/v1/artifact/task/5637156969381888/log/contrib/basebackup_to_shell/tmp_check/log/001_basic_primary.log
From 989258eb413978f342a67e6e3ddd27fecd5dd3d4 Mon Sep 17 00:00:00 2001
From: Robert Haas
Date: Fri, 25 Mar 2022 12:19:44 -0400
Subject: [PATCH 1/2] Tests.
---
contrib/basebackup_to_shell/Makefile | 4 +
contrib/basebackup_to_shell/t/001_basic.pl | 110 +
2 files changed, 114 insertions(+)
create mode 100644 contrib/basebackup_to_shell/t/001_basic.pl
diff --git a/contrib/basebackup_to_shell/Makefile b/contrib/basebackup_to_shell/Makefile
index f31dfaae9c..bbfebcc9a1 100644
--- a/contrib/basebackup_to_shell/Makefile
+++ b/contrib/basebackup_to_shell/Makefile
@@ -7,6 +7,10 @@ OBJS = \
PGFILEDESC = "basebackup_to_shell - target basebackup to shell command"
+TAP_TESTS = 1
+
+export TAR
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/basebackup_to_shell/t/001_basic.pl b/contrib/basebackup_to_shell/t/001_basic.pl
new file mode 100644
index 00..a152fcbc1e
--- /dev/null
+++ b/contrib/basebackup_to_shell/t/001_basic.pl
@@ -0,0 +1,110 @@
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init('allows_streaming' => 1);
+$node->append_conf('postgresql.conf',
+ "shared_preload_libraries = 'basebackup_to_shell'");
+$node->start;
+$node->safe_psql('postgres', 'CREATE USER backupuser REPLICATION');
+$node->safe_psql('postgres', 'CREATE ROLE trustworthy');
+
+# For nearly all pg_basebackup invocations some options should be specified,
+# to keep test times reasonable. Using @pg_basebackup_defs as the first
+# element of the array passed to IPC::Run interpolate the array (as it is
+# not a reference to an array)...
+my @pg_basebackup_defs = ('pg_basebackup', '--no-sync', '-cfast');
+
+# This particular test module generally wants to run with -Xfetch, because
+# -Xstream is not supported with a backup target, and with -U backupuser.
+my @pg_basebackup_cmd = (@pg_basebackup_defs, '-U', 'backupuser', '-Xfetch');
+
+# Can't use this module without setting basebackup_to_shell.command.
+$node->command_fails_like(
+[ @pg_basebackup_cmd, '--target', 'shell' ],
+ qr/shell command for backup is not configured/,
+ 'fails if basebackup_to_shell.command is not set');
+
+# Configure basebackup_to_shell.command and reload the configuation file.
+my $backup_path = PostgreSQL::Test::Utils::tempdir;
+my $shell_command =
+ $PostgreSQL::Test::Utils::windows_os
+ ? qq{type con > "$backup_path%f"} : qq{cat > "$backup_path/%f"};
+$node->append_conf('postgresql.conf',
+ "basebackup_to_shell.command='$shell_command'");
+$node->reload();
+
+# Should work now.
+$node->command_ok(
+[ @pg_basebackup_cmd, '--target', 'shell' ],
+ 'backup with no detail: pg_basebackup');
+verify_backup('', $backup_path, "backup with no detail");
+
+# Should fail with a detail.
+$node->command_fails_like(
+[ @pg_basebackup_cmd, '--target', 'shell:foo' ],
+ qr/a target detail is not permitted because the configured command does not include %d/,
+ 'fails if detail provided without %d');
+
+# Reconfigure to restrict access and require a detail.
+$shell_command =
+ $PostgreSQL::Test::Utils::windows_os
+ ? qq{type con > "$backup_path%d.%f"} : qq{cat > "$backup_path/%d.%f"};
+$node->append_conf('postgresql.conf',
+ "basebackup_to_shell.command='$shell_command'");
+$node->append_conf('postgresql.conf',
+ "basebackup_to_shell.required_role='trustworthy'");
+$node->reload();
+
+# Should fail due to lack of permission.
+$node->command_fails_like(
+[ @pg_basebackup_cmd, '--target', 'shell' ],
+ qr/permission denied to use basebackup_to_shell/,
+ 'fails if required_role not granted');
+
+# Should fail due to lack of a detail.
+$node->safe_psql('postgres', 'GRANT trustworthy TO backupuser');
+$node->command_fails_like(
+[
Re: Document atthasmissing default optimization avoids verification table scan
On Tue, Jan 25, 2022 at 8:49 AM James Coleman wrote: > Here's a version that looks like that. I'm not convinced it's an > improvement over the previous version: again, I expect more advanced > users to already understand this concept, and I think moving it to the > ALTER TABLE page could very well have the effect of burying i(amidst > the ton of detail on the ALTER TABLE page) concept that would be > useful to learn early on in a tutorial like the DDL page. But if > people really think this is an improvement, then I can acquiesce. I vote for rejecting both of these patches. 0001 adds the following sentence to the documentation: "A NOT NULL constraint may be added to the new column in the same statement without requiring scanning the table to verify the constraint." My first reaction when I read this sentence was that it was warning the user about the absence of a hazard that no one would expect in the first place. We could also document that adding a NOT NULL constraint will not cause your gas tank to catch fire, but nobody was worried about that until we brought it up. I also think that the sentence makes the rest of the paragraph harder to understand, because the rest of the paragraph is talking about adding a new column with a default, and now suddenly we're talking about NOT NULL constraints. 0002 moves some advice about adding columns with defaults from one part of the documentation to another. Maybe that's a good idea, and maybe it isn't, but it also rewords the advice, and in my opinion, the new wording is less clear and specific than the existing wording. It also changes a sentence that mentions volatile defaults to give a specific example of a volatile function -- clock_timestamp -- probably because where the documentation was before that function was mentioned. However, that sentence seems clear enough as it is and does not really need an example. I am not trying to pretend that all of our documentation in this area is totally ideal or that nothing can be done to make it better. However, I don't think that these particular patches actually make it better. And I also think that there's only so much time that is worth spending on a patch set like this. Not everything that is confusing about the system is ever going to make its way into the documentation, and that would remain true even if we massively expanded the level of detail that we put in there. That doesn't mean that James or anyone else shouldn't suggest things to add as they find things that they think should be added, but it does mean that not every such suggestion is going to get traction and that's OK too. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error
Fabien COELHO writes: >> [...] One way to avoid these errors is to send Parse messages before >> pipeline mode starts. I attached a patch to fix to prepare commands at >> starting of a script instead of at the first execution of the command. > ISTM that moving prepare out of command execution is a good idea, so I'm > in favor of this approach: the code is simpler and cleaner. > ISTM that a minor impact is that the preparation is not counted in the > command performance statistics. I do not think that it is a problem, even > if it would change detailed results under -C -r -M prepared. I am not convinced this is a great idea. The current behavior is that a statement is not prepared until it's about to be executed, and I think we chose that deliberately to avoid semantic differences between prepared and not-prepared mode. For example, if a script looks like CREATE FUNCTION foo(...) ...; SELECT foo(...); DROP FUNCTION foo; trying to prepare the SELECT in advance would lead to failure. We could perhaps get away with preparing the commands within a pipeline just before we start to execute the pipeline, but it looks to me like this patch tries to prepare the entire script in advance. BTW, the cfbot says the patch is failing to apply anyway ... I think it was sideswiped by 4a39f87ac. regards, tom lane
Re: logical decoding and replication of sequences
Hi, I've fixed most of the reported issues (or at least I think so), with the exception of those in apply_handle_sequence function, i.e.: 1) properly coordinating with the tablesync worker 2) considering skip_lsn, skipping changes 3) missing privilege check, similar to TargetPrivilegesCheck 4) nicer error message if the sequence does not exist The apply_handle_sequence stuff seems to be inter-related, so I plan to deal with that in a single separate commit - the main part being the tablesync coordination, per the fix I shared earlier today. But I need time to think about that, I don't want to rush that. Thanks for the feedback! -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pgsql: Add 'basebackup_to_shell' contrib module.
On 3/25/22 13:52, Robert Haas wrote: > On Fri, Mar 25, 2022 at 12:36 PM Andres Freund wrote: >> Create a CF entry for it, or enable CI on a github repo? > I created a CF entry for it. Then I had to try to Google around to > find the URL from the cfbot, because it's not even linked from > commitfest.postgresql.org for some reason. #blamemagnus > > I don't think that the Windows CI is running the TAP tests for > contrib. At least, I can't find any indication of it in the output. So > it doesn't really help to assess how portable this test is, unless I'm > missing something. > > I looked through the Linux output. It looks to me like that does run > the TAP tests for contrib. Unfortunately, the output is not in order > and is also not labelled, so it's hard to tell what output goes with > what contrib module. I named my test 001_basic.pl, but there are 12 of > those already. I see that 13 copies of 001_basic.pl seem to have > passed CI on Linux, so I guess the test ran and passed there. It seems > like it would be an awfully good idea to mention the subdirectory name > before each dump of output. Duplication of TAP test names has long been something that's annoyed me. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: logical decoding and replication of sequences
On 3/25/22 15:34, vignesh C wrote:
> On Fri, Mar 25, 2022 at 3:29 AM Tomas Vondra
> wrote:
>>
>> Hi,
>>
>> Pushed, after going through the patch once more, addressed the remaining
>> FIXMEs, corrected a couple places in the docs and comments, etc. Minor
>> tweaks, nothing important.
>
> While rebasing patch [1] I found a couple of comments:
> static void
> ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
> -List **rels, List **schemas)
> +List **tables, List **sequences,
> +List **tables_schemas, List **sequences_schemas,
> +List **schemas)
> {
> ListCell *cell;
> PublicationObjSpec *pubobj;
> @@ -185,12 +194,23 @@ ObjectsInPublicationToOids(List
> *pubobjspec_list, ParseState *pstate,
> switch (pubobj->pubobjtype)
> {
> case PUBLICATIONOBJ_TABLE:
> - *rels = lappend(*rels, pubobj->pubtable);
> + *tables = lappend(*tables, pubobj->pubtable);
> + break;
> + case PUBLICATIONOBJ_SEQUENCE:
> + *sequences = lappend(*sequences, pubobj->pubtable);
> break;
> case PUBLICATIONOBJ_TABLES_IN_SCHEMA:
> schemaid = get_namespace_oid(pubobj->name, false);
>
> /* Filter out duplicates if user specifies "sch1, sch1" */
> + *tables_schemas = list_append_unique_oid(*tables_schemas, schemaid);
> + *schemas = list_append_unique_oid(*schemas, schemaid);
> + break;
>
> Now tables_schemas and sequence_schemas are being updated and used in
> ObjectsInPublicationToOids, schema parameter is no longer being used
> after processing in ObjectsInPublicationToOids, I felt we can remove
> that parameter.
>
Thanks! That's a nice simplification, I'll get that pushed in a couple
minutes.
> /* ALTER PUBLICATION ADD */
> else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD"))
> - COMPLETE_WITH("ALL TABLES IN SCHEMA", "TABLE");
> + COMPLETE_WITH("ALL TABLES IN SCHEMA", "ALL SEQUENCES IN SCHEMA",
> "TABLE", "SEQUENCE");
>
> Tab completion of alter publication for ADD and DROP is the same, we
> could combine it.
>
We could, but I find these combined rules harder to read, so I'll keep
the current tab-completion.
> Attached a patch for the same.
> Thoughts?
Thanks for taking a look! Appreciated.
>
> [1] -
> https://www.postgresql.org/message-id/CALDaNm3%3DJrucjhiiwsYQw5-PGtBHFONa6F7hhWCXMsGvh%3DtamA%40mail.gmail.com
>
regars
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: SSL/TLS instead of SSL in docs
On Wed, Sep 15, 2021 at 8:47 AM Daniel Gustafsson wrote: > Since the approach taken wasn't to anyones liking, attached is a v4 (partly > extracted from the previous patch) which only adds notes that SSL is used > interchangeably with TLS in our documentation and configuration. I have actually been wondering why we have been insisting on calling it SSL when it clearly is not. However, if we're not ready/willing to make a bigger change, then doing as you have proposed here seems fine to me. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Accommodate startup process in a separate ProcState array slot instead of in MaxBackends slots.
On Sat, Feb 12, 2022 at 6:26 AM Bharath Rupireddy wrote: > FWIW, here's a patch just adding a comment on how the startup process > can get a free procState array slot even when SInvalShmemSize hasn't > accounted for it. I don't think the positioning of this code comment is very good, because it's commenting on 0 lines of code. Perhaps that problem could be fixed by making it the second paragraph of the immediately preceding comment instead of a separate block, but I think the right place to comment on this sort of thing is actually in the code that sizes the data structure - i.e. SInvalShmemSize. If someone looks at that function and says "hey, this uses GetMaxBackends(), that's off by one!" they are not ever going to find this comment explaining the reasoning. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg14 psql broke \d datname.nspname.relname
> On Mar 22, 2022, at 11:04 AM, Robert Haas wrote:
>
> This patch adds three new arguments to processSQLNamePattern() and
> documents one of them. It adds three new parameters to
> patternToSQLRegex() as well, and documents none of them.
This next patch adds the missing comments.
> I think that
> the text of the comment might need some updating too, in particular
> the sentence "Additional dots in the name portion are not treated as
> special."
Changed.
> There are no comments explaining the left_is_literal stuff. It appears
> that your intention here is that if the pattern string supplied by the
> user contains any of *?|+()[]{}.^\ not surrounded by double-quotes, we
> signal the caller. Some callers then use this to issue a complaint
> that the database name must be a literal. To me, this behavior doesn't
> really make sense. If something is a literal, that means we're not
> going to interpret the special characters that it contains. Here, we
> are interpreting the special characters just so we can complain that
> they exist. It seems to me that a simpler solution would be to not
> interpret them at all. I attach a patch showing what I mean by that.
> It just rips out the dbname_is_literal stuff in favor of doing nothing
> at all. To put the whole thing another way, if the user types "\d
> }.public.ft", your code wants to complain about the fact that the user
> is trying to use regular expression characters in a place where they
> are not allowed to do that. I argue that we should instead just be
> comparing "}" against the database name and see whether it happens to
> match.
I think your change is fine, so I've rolled it into this next patch.
v7-0001-Reject-patterns-with-too-many-parts-or-wrong-db.patch
Description: Binary data
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Add non-blocking version of PQcancel
Robert Haas writes: > Well, that's a fair point, but it's somewhat orthogonal to the one I'm > making, which is that a non-blocking version of function X might be > expected to share code or at least functionality with X itself. Having > something that is named in a way that implies asynchrony without other > differences but which is actually different in other important ways is > no good. Yeah. We need to choose a name for these new function(s) that is sufficiently different from "PQcancel" that people won't expect them to behave exactly the same as that does. I lack any good ideas about that, how about you? >> Yeah, I don't think it's anywhere near fully baked yet. On the other >> hand, we do have a couple of weeks left. > We do? Um, you did read the psql-release discussion about setting the feature freeze deadline, no? regards, tom lane
Re: [PATCH] Enable SSL library detection via PQsslAttribute
On Mon, Feb 28, 2022 at 3:21 PM Jacob Champion wrote: > v3 rebases over Andres' changes and actually adds the Perl driver that > I missed the git-add for. This seems totally reasonable. However, I think it should update the documentation somehow. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add psql command to list constraints
On Fri, Mar 25, 2022 at 3:20 PM Justin Pryzby wrote: > \dX is similar, and I remember wondering whether it was really useful/needed. > The catalog tables are exposed and documented for a reason, and power-users > will learn to use them. I don't think that \dX is comparable, because I don't think we should regard extended statistics as a table object. Indeed, generalizing extended statistics so that they can be generated on a join seems to me to be one of the most important things we could do in that area. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add non-blocking version of PQcancel
On Fri, Mar 25, 2022 at 2:47 PM Tom Lane wrote: > I think you misunderstand where the real pain point is. The reason > that PQcancel's functionality is so limited has little to do with > blocking vs non-blocking, and everything to do with the fact that > it's designed to be safe to call from a SIGINT handler. That makes > it quite impractical to invoke OpenSSL, and probably our GSS code > as well. If we want support for all connection-time options then > we have to make a new function that does not promise signal safety. Well, that's a fair point, but it's somewhat orthogonal to the one I'm making, which is that a non-blocking version of function X might be expected to share code or at least functionality with X itself. Having something that is named in a way that implies asynchrony without other differences but which is actually different in other important ways is no good. > I'm prepared to yield on the question of whether we should provide > a non-blocking version, though I still say that (a) an easier-to-call, > one-step blocking alternative would be good too, and (b) it should > not be designed around the assumption that there's a completely > independent state object being used to perform the cancel. Even in > the non-blocking case, callers should only deal with the original > PGconn. Well, this sounds like you're arguing for the first of the two approaches I thought would be acceptable, rather than the second. > > Leaving the question of approach aside, I think it's fairly clear that > > this patch cannot be seriously considered for v15. > > Yeah, I don't think it's anywhere near fully baked yet. On the other > hand, we do have a couple of weeks left. We do? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add psql command to list constraints
On Fri, Mar 25, 2022 at 03:11:47PM -0400, Robert Haas wrote: > On Fri, Mar 25, 2022 at 12:28 AM Greg Stark wrote: > > Development of this seems to have stalled with the only review of this > > patch expressing some skepticism about whether it's needed at all. > > Now, there is some precedent for the idea of providing a command that > lists everything globally. Specifically, we have a \dp command, also > known as \z, to list privileges across all objects in the database. > The other thing that we have that is somewhat similar to this is \dd, \dX is similar, and I remember wondering whether it was really useful/needed. The catalog tables are exposed and documented for a reason, and power-users will learn to use them. +0.5 to mark the patch as RWF or rejected. -- Justin
Re: Add psql command to list constraints
On Fri, Mar 25, 2022 at 12:28 AM Greg Stark wrote: > Development of this seems to have stalled with the only review of this > patch expressing some skepticism about whether it's needed at all. My opinion on this patch is that we typically handle objects that are essentially table properties by showing the output in \d+ on the individual table. And that already works just fine: rhaas=# create table duck (quack int unique, check (quack > 0)); CREATE TABLE rhaas=# \d+ duck Table "public.duck" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description +-+---+--+-+-+-+--+- quack | integer | | | | plain | | | Indexes: "duck_quack_key" UNIQUE CONSTRAINT, btree (quack) Check constraints: "duck_quack_check" CHECK (quack > 0) Access method: heap Now, there is some precedent for the idea of providing a command that lists everything globally. Specifically, we have a \dp command, also known as \z, to list privileges across all objects in the database. However, I have found that command to be relatively useless, because if you've got any significant number of grants in the database it just produces too much output. I think this would have the same problem. The other thing that we have that is somewhat similar to this is \dd, which lists comments "for object types which do not have their comments displayed by their own backslash commands." However, it says that the object types that it covers are "constraint, operator class, operator family, rule, and trigger," and that list is out of date, because operator classes and families got their own backslash commands two years ago. We could update that, but honestly I can't see anyone being too excited about a command that lists comments on every constraint, rule, and trigger in the system: it would be a lot more useful to show those commands in the \d+ output for the table to which they are bound, and get rid of \dd (and maybe \dp and \z too). Now that is not to say that what is being proposed here is completely useless. It clearly isn't. It's totally debatable whether we ought to start having commands like this, and maybe we should. It would make for more commands, and that's not entirely great because the command names are increasingly alphabet soup. Who can remember what \drds or \dAc does? Only real power users. If we add more, it's going to get even more difficult, but some people will use it and like it and those people will be happy. It's kind of hard to say whether we'd come out ahead or not. What I think is fairly certain is that it would represent a reversal of our current policy. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add non-blocking version of PQcancel
Robert Haas writes: > That said, I don't think that this particular patch is going in the > right direction. I think Jacob's comment upthread is right on point: > "This seems like a big change compared to PQcancel(); one that's not > really hinted at elsewhere. Having the async version of an API open up > a completely different code path with new features is pretty > surprising to me." It seems to me that we want to end up with similar > code paths for PQcancel() and the non-blocking version of cancel. We > could get there in two ways. One way would be to implement the > non-blocking functionality in a manner that matches exactly what > PQcancel() does now. I imagine that the existing code from PQcancel() > would move, with some amount of change, into a new set of non-blocking > APIs. Perhaps PQcancel() would then be rewritten to use those new APIs > instead of hand-rolling the same logic. The other possible approach > would be to first change the blocking version of PQcancel() to use the > regular connection code instead of its own idiosyncratic logic, and > then as a second step, extend it with non-blocking interfaces that use > the regular non-blocking connection code. With either of these > approaches, we end up with the functionality working similarly in the > blocking and non-blocking code paths. I think you misunderstand where the real pain point is. The reason that PQcancel's functionality is so limited has little to do with blocking vs non-blocking, and everything to do with the fact that it's designed to be safe to call from a SIGINT handler. That makes it quite impractical to invoke OpenSSL, and probably our GSS code as well. If we want support for all connection-time options then we have to make a new function that does not promise signal safety. I'm prepared to yield on the question of whether we should provide a non-blocking version, though I still say that (a) an easier-to-call, one-step blocking alternative would be good too, and (b) it should not be designed around the assumption that there's a completely independent state object being used to perform the cancel. Even in the non-blocking case, callers should only deal with the original PGconn. > Leaving the question of approach aside, I think it's fairly clear that > this patch cannot be seriously considered for v15. Yeah, I don't think it's anywhere near fully baked yet. On the other hand, we do have a couple of weeks left. regards, tom lane
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
On Friday, March 25, 2022, Tom Lane wrote: > "David G. Johnston" writes: > > On Fri, Mar 25, 2022 at 10:57 AM Tom Lane wrote: > >> pg_dump never dumps system objects, so I don't see a need for > >> a switch to tell it not to. > > > I considered pg_class to be a system object, which was dumped under -t > '*' > > I'd vote for changing the behavior of --table rather than trying to > be bug-compatible with this decision. > > Agreed. David J.
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
"David G. Johnston" writes: > On Fri, Mar 25, 2022 at 10:57 AM Tom Lane wrote: >> pg_dump never dumps system objects, so I don't see a need for >> a switch to tell it not to. > I considered pg_class to be a system object, which was dumped under -t '*' Oh! You're right, the --table switches will include system objects. That seems like a bug TBH. Even if it's intentional, it's surely not behavior we want for functions. You can somewhat easily exclude system catalogs from matching --table since they all have names starting with "pg_", but it'd be way more painful for functions because (a) there are thousands and (b) they're not very predictably named. I'd vote for changing the behavior of --table rather than trying to be bug-compatible with this decision. regards, tom lane
Re: Add non-blocking version of PQcancel
On Thu, Mar 24, 2022 at 6:49 PM Andres Freund wrote: > That's not a whole lot of fun if you think of cases like postgres_fdw (or > citus as in Jelte's case), which run inside the backend. Even with just a > single postgres_fdw, we don't really want to end up in an uninterruptible > PQcancel() that doesn't even react to pg_terminate_backend(). > > Even if using threads weren't an issue, I don't really buy the premise - most > networking code has moved *away* from using dedicated threads for each > connection. It just doesn't scale. > > Leaving PQcancel aside, we use the non-blocking libpq stuff widely > ourselves. I think walreceiver, isolationtester, pgbench etc would be *much* > harder to get working equally well if there was just blocking calls. If > anything, we're getting to the point where purely blocking functionality > shouldn't be added anymore. +1. I think having a non-blocking version of PQcancel() available is a great idea, and I've wanted it myself. See commit ae9bfc5d65123aaa0d1cca9988037489760bdeae. That said, I don't think that this particular patch is going in the right direction. I think Jacob's comment upthread is right on point: "This seems like a big change compared to PQcancel(); one that's not really hinted at elsewhere. Having the async version of an API open up a completely different code path with new features is pretty surprising to me." It seems to me that we want to end up with similar code paths for PQcancel() and the non-blocking version of cancel. We could get there in two ways. One way would be to implement the non-blocking functionality in a manner that matches exactly what PQcancel() does now. I imagine that the existing code from PQcancel() would move, with some amount of change, into a new set of non-blocking APIs. Perhaps PQcancel() would then be rewritten to use those new APIs instead of hand-rolling the same logic. The other possible approach would be to first change the blocking version of PQcancel() to use the regular connection code instead of its own idiosyncratic logic, and then as a second step, extend it with non-blocking interfaces that use the regular non-blocking connection code. With either of these approaches, we end up with the functionality working similarly in the blocking and non-blocking code paths. Leaving the question of approach aside, I think it's fairly clear that this patch cannot be seriously considered for v15. One problem is the lack of user-facing documentation, but there's a other stuff that just doesn't look sufficiently well-considered. For example, it updates the comment for pqsecure_read() to say "Returns -1 in case of failures, except in the case of clean connection closure then it returns -2." But that function calls any of three different implementation functions depending on the situation and the patch only updates one of them. And it updates that function to return -2 when the is ECONNRESET, which seems to fly in the face of the comment's idea that this is the "clean connection closure" case. I think it's probably a bad sign that this function is tinkering with logic in this sort of low-level function anyway. pqReadData() is a really general function that manages to work with non-blocking I/O already, so why does non-blocking query cancellation need to change its return values, or whether or not it drops data in certain cases? I'm also skeptical about the fact that we end up with a whole bunch of new functions that are just wrappers around existing functions. That's not a scalable approach. Every function that we have for a PGconn will eventually need a variant that deals with a PGcancelConn. That seems kind of pointless, especially considering that a PGcancelConn is *exactly* a PGconn in disguise. If we decide to pursue the approach of using the existing infrastructure for PGconn objects to handle query cancellation, we ought to manipulate them using the same functions we currently do, with some kind of mode or flag or switch or something that you can use to turn a regular PGconn into something that cancels a query. Maybe you create the PGconn and call PQsprinkleMagicCancelDust() on it, and then you just proceed using the existing functions, or something like that. Then, not only do the existing functions not need query-cancel analogues, but any new functions we add in the future don't either. I'll set the target version for this patch to 16. I hope work continues. Thanks, -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
On Fri, Mar 25, 2022 at 10:57 AM Tom Lane wrote: > "David G. Johnston" writes: > > > Except succinctly > > omitting system objects which should get its own general option. > pg_dump never dumps system objects, so I don't see a need for > a switch to tell it not to. > > I considered pg_class to be a system object, which was dumped under -t '*' $ pg_dump -U postgres --schema-only -t '*' | grep 'CREATE.*pg_class' CREATE TABLE pg_catalog.pg_class ( CREATE UNIQUE INDEX pg_class_oid_index ON pg_catalog.pg_class USING btree (oid); CREATE UNIQUE INDEX pg_class_relname_nsp_index ON pg_catalog.pg_class USING btree (relname, relnamespace); CREATE INDEX pg_class_tblspc_relfilenode_index ON pg_catalog.pg_class USING btree (reltablespace, relfilenode); $ psql -U postgres -c 'select version();' version -- PostgreSQL 13.6 (Ubuntu 13.6-1.pgdg20.04+1) on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0, 64-bit (1 row) David J.
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
"David G. Johnston" writes: > I don't find the --objectype-only option to be desirable. psql > --tables-only --functions-only just seems odd, no longer are they "only". > I would go with --function-all (and maybe --function-system and > --function-user) if going down this path but the wildcard feature can > handle this just fine and we want that feature anyway. Agreed. "--function=*" is more general than "--function-only", and shorter too, so what's not to like? > Except succinctly > omitting system objects which should get its own general option. pg_dump never dumps system objects, so I don't see a need for a switch to tell it not to. regards, tom lane
Re: pgsql: Add 'basebackup_to_shell' contrib module.
On Fri, Mar 25, 2022 at 12:36 PM Andres Freund wrote: > Create a CF entry for it, or enable CI on a github repo? I created a CF entry for it. Then I had to try to Google around to find the URL from the cfbot, because it's not even linked from commitfest.postgresql.org for some reason. #blamemagnus I don't think that the Windows CI is running the TAP tests for contrib. At least, I can't find any indication of it in the output. So it doesn't really help to assess how portable this test is, unless I'm missing something. I looked through the Linux output. It looks to me like that does run the TAP tests for contrib. Unfortunately, the output is not in order and is also not labelled, so it's hard to tell what output goes with what contrib module. I named my test 001_basic.pl, but there are 12 of those already. I see that 13 copies of 001_basic.pl seem to have passed CI on Linux, so I guess the test ran and passed there. It seems like it would be an awfully good idea to mention the subdirectory name before each dump of output. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
Laetitia Avrot writes: > Thank you so much for your suggestion. I was really excited to find a > generic term for Functions and Procedures, but "routine" also includes > aggregation functions which I had excluded from my feature (see Postgres > Glossary here: > https://www.postgresql.org/docs/14/glossary.html#GLOSSARY-ROUTINE). > I had decided not to include aggregate functions when I designed my patch > because I thought most users wouldn't expect them in the result file. Was I > wrong? I'd vote for treating them as functions for this purpose. I'd put them in the same category as window functions: we use a separate name for them for historical reasons, but they still walk and quack pretty much like functions. regards, tom lane
Re: Corruption during WAL replay
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= writes: > LGTM, but it would be good to include $! in the die messages. Roger, will do. regards, tom lane
Re: Run end-of-recovery checkpoint in non-wait mode or skip it entirely for faster server availability?
Hi, On March 25, 2022 9:56:38 AM PDT, Robert Haas wrote: >On Fri, Mar 25, 2022 at 3:40 AM Bharath Rupireddy > wrote: >> Since the server spins up checkpointer process [1] while the startup >> process performs recovery, isn't it a good idea to make >> end-of-recovery completely optional for the users or at least run it >> in non-wait mode so that the server will be available faster. The next >> checkpointer cycle will take care of performing the EOR checkpoint >> work, if user chooses to skip the EOR or the checkpointer will run EOR >> checkpoint in background, if user chooses to run it in the non-wait >> mode (without CHECKPOINT_WAIT flag). Of course by choosing this >> option, users must be aware of the fact that the extra amount of >> recovery work that needs to be done if a crash happens from the point >> EOR gets skipped or runs in non-wait mode until the next checkpoint. >> But the advantage that users get is the faster server availability. > >I think that we should remove end-of-recovery checkpoints completely >and instead use the end-of-recovery WAL record (cf. >CreateEndOfRecoveryRecord). However, when I tried to do that, I ran >into some problems: > >http://postgr.es/m/ca+tgmobrm2jvkicccs9ngfcdjnsgavk1qcapx5s6f+ojt3d...@mail.gmail.com > >The second problem described in that email has subsequently been >fixed, I believe, but the first one remains. Seems we could deal with that by making latestCompleted a 64bit xid? Then there never are cases where we have to retreat back into such early xids? A random note from a conversation with Thomas a few days ago: We still perform timeline increases with checkpoints in some cases. Might be worth fixing as a step towards just using EOR. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Re: pg_dump new feature: exporting functions only. Bad or good idea ?
On Fri, Mar 25, 2022 at 9:44 AM Laetitia Avrot wrote: > > Actually, I thought of it after the --schema-only flag (which is kind of > confusing, because it won't export only schema creation DDL). > --schema-only is talking about the different sections of the dump file, not namespace schema objects in the database. > My problem is how do you think we could get all the stored > procedures/functions at once? --function=* ? It seems to me that exporting > everything at once is the main use case (I'd be happy to be proven wrong), > and it does not feel intuitive to use --function=*. > How does one specify "all but only tables" today? If the answer is "we don't" then we get to decide now. I have no qualms with --objecttype=* meaning all. (goes and checks) pg_dump --schema-only -t '*' # indeed outputs all relations. Annoyingly, this seems to include pg_catalog relations as well, so one basically has to specify --exclude-table='pg_catalog.*' as well in the typical case of only wanting user objects. Solving this with a new --no-system-objects that would apply firstly seems like a nice feature to this pre-existing behavior. One might think that --exclude-schema='pg_catalog' would work, but it is doesn't by design. The design choice seems solid for user-space schema names so just dealing with the system objects is my preferred solution. I don't find the --objectype-only option to be desirable. psql --tables-only --functions-only just seems odd, no longer are they "only". I would go with --function-all (and maybe --function-system and --function-user) if going down this path but the wildcard feature can handle this just fine and we want that feature anyway. Except succinctly omitting system objects which should get its own general option. David J.
Re: Run end-of-recovery checkpoint in non-wait mode or skip it entirely for faster server availability?
On Fri, Mar 25, 2022 at 3:40 AM Bharath Rupireddy wrote: > Since the server spins up checkpointer process [1] while the startup > process performs recovery, isn't it a good idea to make > end-of-recovery completely optional for the users or at least run it > in non-wait mode so that the server will be available faster. The next > checkpointer cycle will take care of performing the EOR checkpoint > work, if user chooses to skip the EOR or the checkpointer will run EOR > checkpoint in background, if user chooses to run it in the non-wait > mode (without CHECKPOINT_WAIT flag). Of course by choosing this > option, users must be aware of the fact that the extra amount of > recovery work that needs to be done if a crash happens from the point > EOR gets skipped or runs in non-wait mode until the next checkpoint. > But the advantage that users get is the faster server availability. I think that we should remove end-of-recovery checkpoints completely and instead use the end-of-recovery WAL record (cf. CreateEndOfRecoveryRecord). However, when I tried to do that, I ran into some problems: http://postgr.es/m/ca+tgmobrm2jvkicccs9ngfcdjnsgavk1qcapx5s6f+ojt3d...@mail.gmail.com The second problem described in that email has subsequently been fixed, I believe, but the first one remains. -- Robert Haas EDB: http://www.enterprisedb.com
Re: logical decoding and replication of sequences
On 3/25/22 12:59, Tomas Vondra wrote:
>
> On 3/25/22 12:21, Amit Kapila wrote:
>> On Fri, Mar 25, 2022 at 3:56 PM Tomas Vondra
>> wrote:
>>>
>>>
>>> On 3/25/22 05:01, Amit Kapila wrote:
On Fri, Mar 25, 2022 at 3:29 AM Tomas Vondra
wrote:
>
> Pushed.
>
Some of the comments given by me [1] don't seem to be addressed or
responded to. Let me try to say again for the ease of discussion:
>>>
>>> D'oh! I got distracted by Petr's response to that message, and missed
>>> this part ...
>>>
* Don't we need some syncing mechanism between apply worker and
sequence sync worker so that apply worker skips the sequence changes
till the sync worker is finished, otherwise, there is a risk of one
overriding the values of the other? See how we take care of this for a
table in should_apply_changes_for_rel() and its callers. If we don't
do this for sequences for some reason then probably a comment
somewhere is required.
>>>
>>> How would that happen? If we're effectively setting the sequence as a
>>> side effect of inserting the data, then why should we even replicate the
>>> sequence?
>>>
>>
>> I was talking just about sequence values here, considering that some
>> sequence is just replicating based on nextval. I think the problem is
>> that apply worker might override what copy has done if an apply worker
>> is behind the sequence sync worker as both can run in parallel. Let me
>> try to take some theoretical example to explain this:
>>
>> Assume, at LSN 1, the value of sequence s1 is 10. Then by LSN
>> 12000, the value of s1 becomes 20. Now, say copy decides to copy the
>> sequence value till LSN 12000 which means it will make the value as 20
>> on the subscriber, now, in parallel, apply worker can process LSN
>> 1 and make it again 10. Apply worker might end up redoing all
>> sequence operations along with some transactional ones where we
>> recreate the file. I am not sure what exact problem it can lead to but
>> I think we don't need to redo the work.
>>
>> We'll have the problem later too, no?
>>
>
> Ah, I was confused why this would be an issue for sequences and not for
> plain tables, but now I realize apply_handle_sequence() is not called in
> apply_handle_sequence. Yes, that's probably a thinko.
>
Hmm, so fixing this might be a bit trickier than I expected.
Firstly, currently we only send nspname/relname in the sequence message,
not the remote OID or schema. The idea was that for sequences we don't
really need schema info, so this seemed OK.
But should_apply_changes_for_rel() needs LogicalRepRelMapEntry, and to
create/maintain that those records we need to send the schema.
Attached is a WIP patch does that.
Two places need more work, I think:
1) maybe_send_schema needs ReorderBufferChange, but we don't have that
for sequences, we only have TXN. I created a simple wrapper, but maybe
we should just tweak maybe_send_schema to use TXN.
2) The transaction handling in is a bit confusing. The non-transactional
increments won't have any explicit commit later, so we can't just rely
on begin_replication_step/end_replication_step. But I want to try
spending a bit more time on this.
But there's a more serious issue, I think. So far, we allowed this:
BEGIN;
CREATE SEQUENCE s2;
ALTER PUBLICATION p ADD SEQUENCE s2;
INSERT INTO seq_test SELECT nextval('s2') FROM generate_series(1,100);
COMMIT;
and the behavior was that we replicated the changes. But with the patch
applied, that no longer happens, because should_apply_changes_for_rel
says the change should not be applied.
And after thinking about this, I think that's correct - we can't apply
changes until ALTER SUBSCRIPTION ... REFRESH PUBLICATION gets executed,
and we can't do that until the transaction commits.
So I guess that's correct, and the current behavior is a bug.
For a while I was thinking that maybe this means we don't need the
transactional behavior at all, but I think we do - we have to handle
ALTER SEQUENCE cases that are transactional.
Does that make sense, Amit?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Companydiff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c
index 3dbe85d61a..0ae0378191 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -657,7 +657,6 @@ logicalrep_write_sequence(StringInfo out, Relation rel, TransactionId xid,
int64 last_value, int64 log_cnt, bool is_called)
{
uint8 flags = 0;
- char *relname;
pq_sendbyte(out, LOGICAL_REP_MSG_SEQUENCE);
@@ -668,9 +667,8 @@ logicalrep_write_sequence(StringInfo out, Relation rel, TransactionId xid,
pq_sendint8(out, flags);
pq_sendint64(out, lsn);
- logicalrep_write_namespace(out, RelationGetNamespace(rel));
- relname = RelationGetRelationName(rel);
- pq_sendstring(out, relname);
+ /* OID ad sequence identifier */
+
Re: Re: pg_dump new feature: exporting functions only. Bad or good idea ?
Hello David, thank you for your interest in that patch. Le ven. 25 mars 2022 à 01:17, David G. Johnston a écrit : > On Thu, Mar 24, 2022 at 4:42 PM Chapman Flack > wrote: > >> On 03/27/21 08:57, Andrew Dunstan wrote: >> > We can bikeshed the name of the flag at some stage. --procedures-only >> > might also make sense >> >> Any takers for --routines-only ? >> >> "Routine" is the genuine, ISO SQL umbrella term for a function or >> procedure, and we have used it that way in our docs and glossary. >> >> > Regardless of the prefix name choice neither blobs, tables, nor schemas > use the "-only" suffix so I don't see that this should. I have no issue if > we add three options for this: --routine/--procedure/--function (these are > singular because --table and --schema are singular) > Actually, I thought of it after the --schema-only flag (which is kind of confusing, because it won't export only schema creation DDL). I agree to keep the flag name singular if we're using a pattern to cherry-pick the objects we want. My problem is how do you think we could get all the stored procedures/functions at once? --function=* ? It seems to me that exporting everything at once is the main use case (I'd be happy to be proven wrong), and it does not feel intuitive to use --function=*. How would you see to create 3 flags: --functions-only / --function= / --exclude-function= ? And then we could create the same series of 3 flags for other objects. Would that be too verbose? > > --blobs and --no-blobs are special so let us just build off of the API > already implemented for --table/--exclude-table > > No short option is required, and honestly I don't think it is worthwhile > to take up short options for this, acknowledging that we are leaving -t/-T > (and -n/-N) in place for legacy support. > I agree > > --blobs reacts to these additional object types in the same manner that it > reacts to --table. As soon as any of these object type inclusion options > is specified nothing except the options that are specified will be output. > Both data and schema, though, for most object types, data is not relevant. > If schema is not output then options that control schema content objects > only are ignored. > > The --exclude-* options behave in the same way as defined for -t/-T, > specifically the note in -T about when both are present. > > As with tables, the affirmative version of these overrides any --schema > (-n/-N) specification provided. But the --exclude-* versions of these do > omit the named objects from the dump should they have been selected by > --schema. > That's fine with me. Have a nice day, Lætitia > > David J. > >
Re: pgsql: Add 'basebackup_to_shell' contrib module.
Hi, On March 25, 2022 9:22:09 AM PDT, Robert Haas wrote: >On Thu, Mar 17, 2022 at 11:52 AM Robert Haas wrote: >> On Tue, Mar 15, 2022 at 3:04 PM Andres Freund wrote: >> > Seems like this ought to have at least some basic test to make sure it >> > actually works / keeps working? >> >> Wouldn't hurt, although it may be a little bit tricky to getting it >> work portably. I'll try to take a look at it. > >Here is a basic test. I am unable to verify whether it works on Windows. Create a CF entry for it, or enable CI on a github repo? Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Re: pg_dump new feature: exporting functions only. Bad or good idea ?
Hello Chapman, Le ven. 25 mars 2022 à 00:42, Chapman Flack a écrit : > On 03/27/21 08:57, Andrew Dunstan wrote: > > We can bikeshed the name of the flag at some stage. --procedures-only > > might also make sense > > Any takers for --routines-only ? > > "Routine" is the genuine, ISO SQL umbrella term for a function or > procedure, and we have used it that way in our docs and glossary. > Thank you so much for your suggestion. I was really excited to find a generic term for Functions and Procedures, but "routine" also includes aggregation functions which I had excluded from my feature (see Postgres Glossary here: https://www.postgresql.org/docs/14/glossary.html#GLOSSARY-ROUTINE). I had decided not to include aggregate functions when I designed my patch because I thought most users wouldn't expect them in the result file. Was I wrong? Have a nice day, Lætitia > > Regards, > -Chap >
Re: Corruption during WAL replay
Tom Lane writes:
> Robert Haas writes:
>> ... It's not
>> like a 16-bit checksum was state-of-the-art even when we introduced
>> it. We just did it because we had 2 bytes that we could repurpose
>> relatively painlessly, and not any larger number. And that's still the
>> case today, so at least in the short term we will have to choose some
>> other solution to this problem.
>
> Indeed. I propose the attached, which also fixes the unsafe use
> of seek() alongside syswrite(), directly contrary to what "man perlfunc"
> says to do.
LGTM, but it would be good to include $! in the die messages.
- ilmari
> regards, tom lane
>
> diff --git a/src/bin/pg_checksums/t/002_actions.pl
> b/src/bin/pg_checksums/t/002_actions.pl
> index 62c608eaf6..8c70453a45 100644
> --- a/src/bin/pg_checksums/t/002_actions.pl
> +++ b/src/bin/pg_checksums/t/002_actions.pl
> @@ -24,6 +24,7 @@ sub check_relation_corruption
> my $tablespace = shift;
> my $pgdata = $node->data_dir;
>
> + # Create table and discover its filesystem location.
> $node->safe_psql(
> 'postgres',
> "CREATE TABLE $table AS SELECT a FROM generate_series(1,1)
> AS a;
> @@ -37,9 +38,6 @@ sub check_relation_corruption
> my $relfilenode_corrupted = $node->safe_psql('postgres',
> "SELECT relfilenode FROM pg_class WHERE relname = '$table';");
>
> - # Set page header and block size
> - my $pageheader_size = 24;
> - my $block_size = $node->safe_psql('postgres', 'SHOW block_size;');
> $node->stop;
>
> # Checksums are correct for single relfilenode as the table is not
> @@ -55,8 +53,12 @@ sub check_relation_corruption
>
> # Time to create some corruption
> open my $file, '+<', "$pgdata/$file_corrupted";
> - seek($file, $pageheader_size, SEEK_SET);
> - syswrite($file, "\0\0\0\0\0\0\0\0\0");
> + my $pageheader;
> + sysread($file, $pageheader, 24) or die "sysread failed";
> + # This inverts the pd_checksum field (only); see struct PageHeaderData
> + $pageheader ^= "\0\0\0\0\0\0\0\0\xff\xff";
> + sysseek($file, 0, 0) or die "sysseek failed";
> + syswrite($file, $pageheader) or die "syswrite failed";
> close $file;
>
> # Checksum checks on single relfilenode fail
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
Hello Tom, Le ven. 25 mars 2022 à 00:18, Tom Lane a écrit : > Daniel Gustafsson writes: > >> On 24 Mar 2022, at 23:38, Greg Stark wrote: > >> It looks like this discussion has reached a bit of an impasse with Tom > >> being against this approach and Michael and Daniel being for it. It > >> doesn't look like it's going to get committed this commitfest, shall > >> we move it forward or mark it returned with feedback? > > > Lætitia mentioned the other day off-list that she was going to try and > update > > this patch with the pattern support proposed, so hopefully we will hear > from > > her shortly on that. > > To clarify: I'm not against having an easy way to dump all-and-only > functions. What concerns me is having such a feature that's designed > in isolation, without a plan for anything else. I'd like to create > some sort of road map for future selective-dumping options, and then > we can make sure that this feature fits into the bigger picture. > Otherwise we're going to end up with an accumulation of warts, with > inconsistent naming and syntax, and who knows what other sources of > confusion. > This totally makes sense. Have a nice day, Lætitia > > regards, tom lane >
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
Hello Michael, Le mar. 25 janv. 2022 à 06:49, Michael Paquier a écrit : > On Tue, Nov 09, 2021 at 03:23:07PM +0100, Daniel Gustafsson wrote: > > Looking at this thread I think it makes sense to go ahead with this > patch. The > > filter functionality worked on in another thread is dealing with > cherry-picking > > certain objects where this is an all-or-nothing switch, so I don't think > they > > are at odds with each other. > > Including both procedures and functions sounds natural from here. Now > I have a different question, something that has not been discussed in > this thread at all. What about patterns? Switches like --table or > --extension are able to digest a psql-like pattern to decide which > objects to dump. Is there a reason not to have this capability for > this new switch with procedure names? I mean to handle the case > without the function arguments, even if the same name is used by > multiple functions with different arguments. > Thank you for this suggestion. We have --schema-only flag to export only the structure and then we have --schema= flag to export the schemas following a pattern. I don't think both features can't exist for functions (and stored procedures), but I see them as different features. We could have --functions-only and --function=. In my humble opinion, the lack of --function= feature should block this patch. Have a great day, Lætitia > -- > Michael >
Re: pgsql: Add 'basebackup_to_shell' contrib module.
On Thu, Mar 17, 2022 at 11:52 AM Robert Haas wrote: > On Tue, Mar 15, 2022 at 3:04 PM Andres Freund wrote: > > Seems like this ought to have at least some basic test to make sure it > > actually works / keeps working? > > Wouldn't hurt, although it may be a little bit tricky to getting it > work portably. I'll try to take a look at it. Here is a basic test. I am unable to verify whether it works on Windows. -- Robert Haas EDB: http://www.enterprisedb.com 0001-Tests.patch Description: Binary data
Re: Corruption during WAL replay
Andres Freund writes: > The same code also exists in src/bin/pg_basebackup/t/010_pg_basebackup.pl, > which presumably has the same collision risks. Oooh, I missed that. > Perhaps we should put a > function into Cluster.pm and use it from both? +1, I'll make it so. regards, tom lane
Re: Corruption during WAL replay
Hi, On 2022-03-25 11:50:48 -0400, Tom Lane wrote: > Robert Haas writes: > > ... It's not > > like a 16-bit checksum was state-of-the-art even when we introduced > > it. We just did it because we had 2 bytes that we could repurpose > > relatively painlessly, and not any larger number. And that's still the > > case today, so at least in the short term we will have to choose some > > other solution to this problem. > > Indeed. I propose the attached, which also fixes the unsafe use > of seek() alongside syswrite(), directly contrary to what "man perlfunc" > says to do. That looks reasonable. Although I wonder if we loose something by not testing the influence of the rest of the block - but I don't really see anything. The same code also exists in src/bin/pg_basebackup/t/010_pg_basebackup.pl, which presumably has the same collision risks. Perhaps we should put a function into Cluster.pm and use it from both? Greetings, Andres Freund
Re: Probable memory leak with ECPG and AIX
Le ven. 25 mars 2022, 14:25, Tom Lane a écrit : > Guillaume Lelarge writes: > > Did this get anywhere? Is there something we could do to make this move > > forward? > > No. Write a patch? > I wouldn't have asked if I could write such a patch :-)
Re: ubsan
On 3/23/22 16:55, Andres Freund wrote: It's particularly impressive that the cost of running with ASAN is *so* much lower than valgrind. On my workstation a check-world with -fsanitize=alignment,undefined,address takes 3min17s, vs 1min10s or so without -fsanitize. Not something to always use, but certainly better than valgrind. It also catches things that valgrind does not so that's a bonus. One thing to note, though. I have noticed that when enabling -fsanitize=undefined and/or -fsanitize=address in combination with -fprofile-arcs -ftest-coverage there is a loss in reported coverage, at least on gcc 9.3. This may not be very obvious unless coverage is normally at 100%. Regards, -David
Re: Corruption during WAL replay
Robert Haas writes:
> ... It's not
> like a 16-bit checksum was state-of-the-art even when we introduced
> it. We just did it because we had 2 bytes that we could repurpose
> relatively painlessly, and not any larger number. And that's still the
> case today, so at least in the short term we will have to choose some
> other solution to this problem.
Indeed. I propose the attached, which also fixes the unsafe use
of seek() alongside syswrite(), directly contrary to what "man perlfunc"
says to do.
regards, tom lane
diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl
index 62c608eaf6..8c70453a45 100644
--- a/src/bin/pg_checksums/t/002_actions.pl
+++ b/src/bin/pg_checksums/t/002_actions.pl
@@ -24,6 +24,7 @@ sub check_relation_corruption
my $tablespace = shift;
my $pgdata = $node->data_dir;
+ # Create table and discover its filesystem location.
$node->safe_psql(
'postgres',
"CREATE TABLE $table AS SELECT a FROM generate_series(1,1) AS a;
@@ -37,9 +38,6 @@ sub check_relation_corruption
my $relfilenode_corrupted = $node->safe_psql('postgres',
"SELECT relfilenode FROM pg_class WHERE relname = '$table';");
- # Set page header and block size
- my $pageheader_size = 24;
- my $block_size = $node->safe_psql('postgres', 'SHOW block_size;');
$node->stop;
# Checksums are correct for single relfilenode as the table is not
@@ -55,8 +53,12 @@ sub check_relation_corruption
# Time to create some corruption
open my $file, '+<', "$pgdata/$file_corrupted";
- seek($file, $pageheader_size, SEEK_SET);
- syswrite($file, "\0\0\0\0\0\0\0\0\0");
+ my $pageheader;
+ sysread($file, $pageheader, 24) or die "sysread failed";
+ # This inverts the pd_checksum field (only); see struct PageHeaderData
+ $pageheader ^= "\0\0\0\0\0\0\0\0\xff\xff";
+ sysseek($file, 0, 0) or die "sysseek failed";
+ syswrite($file, $pageheader) or die "syswrite failed";
close $file;
# Checksum checks on single relfilenode fail
Re: psql - add SHOW_ALL_RESULTS option
As Tom said earlier, wasn't this fixed by 618c16707? If not, is there any other discussion on the specifics of this issue? I'm not aware of one. Hmmm… I'll try to understand why the doubled message seems to be still there. -- Fabien.
Re: pg_relation_size on partitioned table
On Fri, Mar 25, 2022 at 08:52:40PM +0800, Japin Li wrote: > When I try to get total size of partition tables though partitioned table > name using pg_relation_size(), it always returns zero. I can use the > following SQL to get total size of partition tables, however, it is a bit > complex. This doesn't handle multiple levels of partitioning, as \dP+ already does. Any new function should probably be usable by \dP+ (although it would also need to support older server versions for another ~10 years). > SELECT pg_size_pretty(sum(pg_relation_size(i.inhrelid))) > FROM pg_class c JOIN pg_inherits i ON c.oid = i.inhparent > WHERE relname = 'parent'; > Could we provide a function to get the total size of the partition table > though the partitioned table name? Maybe we can extend > the pg_relation_size() to get the total size of partition tables through > the partitioned table name. Sometimes people would want the size of the table itself and not the size of its partitions, so it's not good to change pg_relation_size(). OTOH, pg_total_relation_size() shows a table size including toast and indexes. Toast are an implementation detail, which is intended to be hidden from application developers. And that's a goal for partitioning, too. So maybe it would make sense if it showed the size of the table, toast, indexes, *and* partitions (but not legacy inheritance children). I know I'm not the only one who can't keep track of what all the existing pg_*_size functions include, so adding more functions will also add some additional confusion, unless, perhaps, it took arguments indicating what to include, like pg_total_relation_size(partitions=>false, toast=>true, indexes=>true, fork=>main). -- Justin
Re: automatically generating node support functions
On 25.03.22 14:32, Tom Lane wrote: Peter Eisentraut writes: On 24.03.22 22:57, David Rowley wrote: Also, I'm quite keen to see this work make it into v15. Do you think you'll get time to do that? Thanks for working on it. My thinking right now is to wait for the PG16 branch to open and then consider putting it in early. +1. However, as noted by David (and I think I made similar points awhile ago), the patch could still use a lot of mop-up work. It'd be prudent to continue working on it so it will actually be ready to go when the branch is made. The v5 patch was intended to address all the comments you made in your Feb. 14 mail. I'm not aware of any open issues from that.
Re: Use JOIN USING aliases in ruleutils.c
On 23.03.22 16:14, Tom Lane wrote: My big fear is that it will reduce portability of pg_dump output: views that would have loaded successfully into pre-v14 servers no longer will, and your chances of porting them to other RDBMSes probably go down too. Once v13 becomes EOL, that will be less of a concern, but I think it's a valid objection for awhile yet. That's a good point. I'll withdraw the patch for now.
Re: pg_walinspect - a new extension to get raw WAL data and WAL stats
Hi Bharath,
First look at the patch, bear with me if any of the following comments are
repeated.
1. With pg_get_wal_record(lsn), say a WAL record start, end lsn range
contains the specified LSN, wouldn't it be more meaningful to show the
corresponding WAL record.
For example, upon providing '0/17335E7' as input, and I see get the WAL
record ('0/1733618', '0/173409F') as output and not the one with start and
end lsn as ('0/17335E0', '0/1733617').
With pg_walfile_name(lsn), we can find the WAL segment file name that
contains the specified LSN.
2. I see the following output for pg_get_wal_record. Need to have a look at
the spaces I suppose.
rkn=# select * from pg_get_wal_record('0/4041728');
start_lsn | end_lsn | prev_lsn | record_length |
record
---+---+---+---+-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
Re: psql - add SHOW_ALL_RESULTS option
On 23.03.22 13:58, Fabien COELHO wrote: If you want to wait for libpq to provide a solution for this corner case, I'm afraid that "never" is the likely result, especially as no test case exercices this path to show that there is a problem somewhere, so nobody should care to fix it. I'm not sure it is even worth it given the highly special situation which triggers the issue, which is not such an actual problem (ok, the user is told twice that there was a connection loss, no big deal). As Tom said earlier, wasn't this fixed by 618c16707? If not, is there any other discussion on the specifics of this issue? I'm not aware of one.
Re: Add index scan progress to pg_stat_progress_vacuum
On Wed, Mar 23, 2022 at 6:57 AM Imseih (AWS), Sami wrote: > > >Can the leader pass a callback that checks PVIndStats to ambulkdelete > >an amvacuumcleanup callbacks? I think that in the passed callback, the > >leader checks if the number of processed indexes and updates its > >progress information if the current progress needs to be updated. > > Thanks for the suggestion. > > I looked at this option a but today and found that passing the callback > will also require signature changes to the ambulkdelete and > amvacuumcleanup routines. I think it would not be a critical problem since it's a new feature. > > This will also require us to check after x pages have been > scanned inside vacuumscan and vacuumcleanup. After x pages > the callback can then update the leaders progress. > I am not sure if adding additional complexity to the scan/cleanup path > is justified for what this patch is attempting to do. > > There will also be a lag of the leader updating the progress as it > must scan x amount of pages before updating. Obviously, the more > Pages to the scan, the longer the lag will be. Fair points. On the other hand, the approach of the current patch requires more memory for progress tracking, which could fail, e.g., due to running out of hashtable entries. I think that it would be worse that the parallel operation failed to start due to not being able to track the progress than the above concerns you mentioned such as introducing additional complexity and a possible lag of progress updates. So if we go with the current approach, I think we need to make sure enough (and not too many) hash table entries. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Corruption during WAL replay
On Fri, Mar 25, 2022 at 10:34 AM Tom Lane wrote: > I dunno. Compatibility and speed concerns aside, that seems like an awful > lot of bits to be expending on every page compared to the value. I dunno either, but over on the TDE thread people seemed quite willing to expend like 16-32 *bytes* for page verifiers and nonces and things. For compatibility and speed reasons, I doubt we could ever get by with doing that in every cluster, but I do have some hope of introducing something like that someday at least as an optional feature. It's not like a 16-bit checksum was state-of-the-art even when we introduced it. We just did it because we had 2 bytes that we could repurpose relatively painlessly, and not any larger number. And that's still the case today, so at least in the short term we will have to choose some other solution to this problem. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_relation_size on partitioned table
On Fri, 25 Mar 2022 at 21:21, Bharath Rupireddy wrote: > On Fri, Mar 25, 2022 at 6:23 PM Japin Li wrote: >> >> Hi, hackers >> >> When I try to get total size of partition tables though partitioned table >> name using pg_relation_size(), it always returns zero. I can use the >> following SQL to get total size of partition tables, however, it is a bit >> complex. >> >> SELECT >> pg_size_pretty(sum(pg_relation_size(i.inhrelid))) >> FROM >> pg_class c JOIN pg_inherits i ON c.oid = i.inhparent >> WHERE >> relname = 'parent'; >> >> Could we provide a function to get the total size of the partition table >> though the partitioned table name? Maybe we can extend >> the pg_relation_size() to get the total size of partition tables through >> the partitioned table name. > > If we want to have it in the core, why can't it just be a function (in > system_functions.sql) something like below? Not everyone, would know > how to get partition relation size, especially whey they are not using > psql, they can't use the short forms that it provides. > > CREATE OR REPLACE FUNCTION pg_partition_relation_size(regclass) > RETURNS bigint > LANGUAGE sql > PARALLEL SAFE STRICT COST 1 > BEGIN ATOMIC > SELECT > pg_size_pretty(sum(pg_relation_size(i.inhrelid))) > FROM > pg_class c JOIN pg_inherits i ON c.oid = i.inhparent > WHERE > relname = '$1'; > END; > I add two functions (as suggested by Bharath Rupireddy) pg_partition_relation_size and pg_partition_table_size to get partition tables size through partitioned table name. It may reduce the complexity to get the size of partition tables. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd. diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql index 81bac6f581..81cab4c21c 100644 --- a/src/backend/catalog/system_functions.sql +++ b/src/backend/catalog/system_functions.sql @@ -281,6 +281,32 @@ CREATE OR REPLACE FUNCTION pg_relation_size(regclass) PARALLEL SAFE STRICT COST 1 RETURN pg_relation_size($1, 'main'); +CREATE OR REPLACE FUNCTION pg_partition_relation_size(regclass, text default 'main') +RETURNS bigint +LANGUAGE sql +PARALLEL SAFE STRICT COST 1 +BEGIN ATOMIC +SELECT +sum(pg_relation_size(i.inhrelid, $2)) +FROM +pg_class c JOIN pg_inherits i ON c.oid = i.inhparent +WHERE +oid = $1; +END; + +CREATE OR REPLACE FUNCTION pg_partition_table_size(regclass) +RETURNS bigint +LANGUAGE sql +PARALLEL SAFE STRICT COST 1 +BEGIN ATOMIC +SELECT +sum(pg_table_size(i.inhrelid)) +FROM +pg_class c JOIN pg_inherits i ON c.oid = i.inhparent +WHERE +oid = $1; +END; + CREATE OR REPLACE FUNCTION obj_description(oid, name) RETURNS text LANGUAGE sql
Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints
On Fri, Mar 25, 2022 at 7:41 PM Robert Haas wrote: > > On Thu, Mar 24, 2022 at 12:12 PM Dilip Kumar wrote: > > Thanks, I have gone through your changes in comments and docs and those > > LGTM. > > It looks like this patch will need to be updated for Alvaro's commit > 49d9cfc68bf4e0d32a948fe72d5a0ef7f464944e. The newly added test > 029_replay_tsp_drops.pl fails with this patch applied. The standby log > shows: > > 2022-03-25 10:00:10.022 EDT [38209] LOG: entering standby mode > 2022-03-25 10:00:10.024 EDT [38209] LOG: redo starts at 0/328 > 2022-03-25 10:00:10.062 EDT [38209] FATAL: could not create directory > "pg_tblspc/16385/PG_15_202203241/16390": No such file or directory > 2022-03-25 10:00:10.062 EDT [38209] CONTEXT: WAL redo at 0/43EBD88 > for Database/CREATE_WAL_LOG: create dir 16385/16390 > > On a quick look, I'm guessing that XLOG_DBASE_CREATE_WAL_LOG will need > to mirror some of the logic that was added to the replay code for the > existing strategy, but I haven't figured out the details. > Yeah, I think I got it, for XLOG_DBASE_CREATE_WAL_LOG now we will have to handle the missing parent directory case, like Alvaro handled for the XLOG_DBASE_CREATE(_FILE_COPY) case. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: logical decoding and replication of sequences
On Fri, Mar 25, 2022 at 3:29 AM Tomas Vondra
wrote:
>
> Hi,
>
> Pushed, after going through the patch once more, addressed the remaining
> FIXMEs, corrected a couple places in the docs and comments, etc. Minor
> tweaks, nothing important.
While rebasing patch [1] I found a couple of comments:
static void
ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
-List **rels, List **schemas)
+List **tables, List **sequences,
+List **tables_schemas, List **sequences_schemas,
+List **schemas)
{
ListCell *cell;
PublicationObjSpec *pubobj;
@@ -185,12 +194,23 @@ ObjectsInPublicationToOids(List
*pubobjspec_list, ParseState *pstate,
switch (pubobj->pubobjtype)
{
case PUBLICATIONOBJ_TABLE:
- *rels = lappend(*rels, pubobj->pubtable);
+ *tables = lappend(*tables, pubobj->pubtable);
+ break;
+ case PUBLICATIONOBJ_SEQUENCE:
+ *sequences = lappend(*sequences, pubobj->pubtable);
break;
case PUBLICATIONOBJ_TABLES_IN_SCHEMA:
schemaid = get_namespace_oid(pubobj->name, false);
/* Filter out duplicates if user specifies "sch1, sch1" */
+ *tables_schemas = list_append_unique_oid(*tables_schemas, schemaid);
+ *schemas = list_append_unique_oid(*schemas, schemaid);
+ break;
Now tables_schemas and sequence_schemas are being updated and used in
ObjectsInPublicationToOids, schema parameter is no longer being used
after processing in ObjectsInPublicationToOids, I felt we can remove
that parameter.
/* ALTER PUBLICATION ADD */
else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD"))
- COMPLETE_WITH("ALL TABLES IN SCHEMA", "TABLE");
+ COMPLETE_WITH("ALL TABLES IN SCHEMA", "ALL SEQUENCES IN SCHEMA",
"TABLE", "SEQUENCE");
Tab completion of alter publication for ADD and DROP is the same, we
could combine it.
Attached a patch for the same.
Thoughts?
[1] -
https://www.postgresql.org/message-id/CALDaNm3%3DJrucjhiiwsYQw5-PGtBHFONa6F7hhWCXMsGvh%3DtamA%40mail.gmail.com
Regards,
Vignesh
From dc707ba93494e3ba0cffd8ab6e1fb1b7a1c0e70e Mon Sep 17 00:00:00 2001
From: Vigneshwaran C
Date: Fri, 25 Mar 2022 19:49:40 +0530
Subject: [PATCH] Removed unused parameter from ObjectsInPublicationToOids.
Removed unused parameter from ObjectsInPublicationToOids.
---
src/backend/commands/publicationcmds.c | 15 +++
src/bin/psql/tab-complete.c| 5 +
2 files changed, 4 insertions(+), 16 deletions(-)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index c6437799c5..89298d7857 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -175,8 +175,7 @@ parse_publication_options(ParseState *pstate,
static void
ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
List **tables, List **sequences,
- List **tables_schemas, List **sequences_schemas,
- List **schemas)
+ List **tables_schemas, List **sequences_schemas)
{
ListCell *cell;
PublicationObjSpec *pubobj;
@@ -204,14 +203,12 @@ ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
/* Filter out duplicates if user specifies "sch1, sch1" */
*tables_schemas = list_append_unique_oid(*tables_schemas, schemaid);
-*schemas = list_append_unique_oid(*schemas, schemaid);
break;
case PUBLICATIONOBJ_SEQUENCES_IN_SCHEMA:
schemaid = get_namespace_oid(pubobj->name, false);
/* Filter out duplicates if user specifies "sch1, sch1" */
*sequences_schemas = list_append_unique_oid(*sequences_schemas, schemaid);
-*schemas = list_append_unique_oid(*schemas, schemaid);
break;
case PUBLICATIONOBJ_TABLES_IN_CUR_SCHEMA:
search_path = fetch_search_path(false);
@@ -225,7 +222,6 @@ ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
/* Filter out duplicates if user specifies "sch1, sch1" */
*tables_schemas = list_append_unique_oid(*tables_schemas, schemaid);
-*schemas = list_append_unique_oid(*schemas, schemaid);
break;
case PUBLICATIONOBJ_SEQUENCES_IN_CUR_SCHEMA:
search_path = fetch_search_path(false);
@@ -239,7 +235,6 @@ ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
/* Filter out duplicates if user specifies "sch1, sch1" */
*sequences_schemas = list_append_unique_oid(*sequences_schemas, schemaid);
-*schemas = list_append_unique_oid(*schemas, schemaid);
break;
default:
/* shouldn't happen */
@@ -679,7 +674,6 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)
List *sequences = NIL;
List *tables_schemaidlist = NIL;
List *sequences_schemaidlist = NIL;
- List *schemaidlist = NIL;
bool for_all_tables = false;
bool for_all_sequences = false;
@@ -782,8 +776,7 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)
ObjectsInPublicationToOids(stmt->pubobjects, pstate,
, ,
_schemaidlist,
- _schemaidlist,
- );
+ _schemaidlist);
Re: Corruption during WAL replay
Robert Haas writes: > On Fri, Mar 25, 2022 at 10:02 AM Tom Lane wrote: >> Adding another 16 bits won't get you to that, sadly. Yeah, it *might* >> extend the MTTF to more than the project's likely lifespan, but that >> doesn't mean we couldn't get unlucky next week. > I suspect that the number of bits Andres wants to add is no less than 48. I dunno. Compatibility and speed concerns aside, that seems like an awful lot of bits to be expending on every page compared to the value. regards, tom lane
Re: pg_relation_size on partitioned table
On Fri, Mar 25, 2022 at 6:23 PM Japin Li wrote: > > Hi, hackers > > When I try to get total size of partition tables though partitioned table > name using pg_relation_size(), it always returns zero. I can use the > following SQL to get total size of partition tables, however, it is a bit > complex. > > SELECT > pg_size_pretty(sum(pg_relation_size(i.inhrelid))) > FROM > pg_class c JOIN pg_inherits i ON c.oid = i.inhparent > WHERE > relname = 'parent'; > > Could we provide a function to get the total size of the partition table > though the partitioned table name? Maybe we can extend > the pg_relation_size() to get the total size of partition tables through > the partitioned table name. If we want to have it in the core, why can't it just be a function (in system_functions.sql) something like below? Not everyone, would know how to get partition relation size, especially whey they are not using psql, they can't use the short forms that it provides. CREATE OR REPLACE FUNCTION pg_partition_relation_size(regclass) RETURNS bigint LANGUAGE sql PARALLEL SAFE STRICT COST 1 BEGIN ATOMIC SELECT pg_size_pretty(sum(pg_relation_size(i.inhrelid))) FROM pg_class c JOIN pg_inherits i ON c.oid = i.inhparent WHERE relname = '$1'; END; Regards, Bharath Rupireddy.
Re: Corruption during WAL replay
On Fri, Mar 25, 2022 at 10:02 AM Tom Lane wrote: > Robert Haas writes: > > On Fri, Mar 25, 2022 at 9:49 AM Tom Lane wrote: > >> That'll just reduce the probability of failure, not eliminate it. > > > I mean, if the expected time to the first failure on even 1 machine > > exceeds the time until the heat death of the universe by 10 orders of > > magnitude, it's probably good enough. > > Adding another 16 bits won't get you to that, sadly. Yeah, it *might* > extend the MTTF to more than the project's likely lifespan, but that > doesn't mean we couldn't get unlucky next week. I suspect that the number of bits Andres wants to add is no less than 48. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements
Hi On Fri, 2022-03-25 at 00:37 -0400, Greg Stark wrote: > Fwiw I find the idea of having a separate "aux" table kind of > awkward. > It'll seem strange to users not familiar with the history and without > any clear idea why the fields are split. Greg, thank you for your attention and for your thought. I've just completed the 6th version of a patch implementing idea proposed by Julien Rouhaud, i.e. without auxiliary statistics. 6th version will reset current min/max fields to zeros until the first plan or execute. I've decided to use zeros here because planning statistics is zero in case of disabled tracking. I think sampling solution could easily handle this. -- Regards, Andrei Zubkov From 68cd5efee7b3dbdb1b4034ab4c47249a23ca9d04 Mon Sep 17 00:00:00 2001 From: Andrei Zubkov Date: Fri, 25 Mar 2022 12:30:03 +0300 Subject: [PATCH] pg_stat_statements: Track statement entry timestamp This patch adds stats_since column to the pg_stat_statements view. This column is populated with the current timestamp when a new statement is added to the pg_stat_statements hashtable. It provides clean information about statistics collection time interval for each statement. Besides it can be used by sampling solutions to detect situations when a statement was evicted and returned back between samples. Such sampling solution could derive any pg_stat_statements statistic value for an interval between two samples with except of all min/max statistics. To address this issue this patch adds the ability to reset min/max statistics independently of statement reset using the new function pg_stat_statements_aux_reset(userid oid, dbid oid, queryid bigint). Timestamp of such reset is stored in the minmax_stats_since field for each statement. Discussion: https://www.postgresql.org/message-id/flat/72e80e7b160a6eb189df9ef6f068cce3765d37f8.camel%40moonset.ru --- contrib/pg_stat_statements/Makefile | 3 +- .../expected/pg_stat_statements.out | 140 + .../pg_stat_statements--1.9--1.10.sql | 104 + .../pg_stat_statements/pg_stat_statements.c | 197 -- .../pg_stat_statements.control| 2 +- .../sql/pg_stat_statements.sql| 92 doc/src/sgml/pgstatstatements.sgml| 64 +- 7 files changed, 580 insertions(+), 22 deletions(-) create mode 100644 contrib/pg_stat_statements/pg_stat_statements--1.9--1.10.sql diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile index 7fabd96f38..edc40c8bbf 100644 --- a/contrib/pg_stat_statements/Makefile +++ b/contrib/pg_stat_statements/Makefile @@ -6,7 +6,8 @@ OBJS = \ pg_stat_statements.o EXTENSION = pg_stat_statements -DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.8--1.9.sql \ +DATA = pg_stat_statements--1.4.sql \ + pg_stat_statements--1.9--1.10.sql pg_stat_statements--1.8--1.9.sql \ pg_stat_statements--1.7--1.8.sql pg_stat_statements--1.6--1.7.sql \ pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \ pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \ diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index e0abe34bb6..d59fcdc403 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -1077,4 +1077,144 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%'; 2 (1 row) +-- +-- statement timestamps +-- +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT 1 AS "STMTTS1"; + STMTTS1 +- + 1 +(1 row) + +SELECT now() AS ref_ts \gset +SELECT 1,2 AS "STMTTS2"; + ?column? | STMTTS2 +--+- +1 | 2 +(1 row) + +SELECT stats_since >= :'ref_ts', count(*) FROM pg_stat_statements +WHERE query LIKE '%STMTTS%' +GROUP BY stats_since >= :'ref_ts' +ORDER BY stats_since >= :'ref_ts'; + ?column? | count +--+--- + f| 1 + t| 1 +(2 rows) + +SELECT now() AS ref_ts \gset +SELECT + count(*) as total, + count(*) FILTER ( +WHERE min_plan_time + max_plan_time = 0 + ) as minmax_plan_zero, + count(*) FILTER ( +WHERE min_exec_time + max_exec_time = 0 + ) as minmax_exec_zero, + count(*) FILTER ( +WHERE minmax_stats_since >= :'ref_ts' + ) as minmax_stats_since_after_ref, + count(*) FILTER ( +WHERE stats_since >= :'ref_ts' + ) as stats_since_after_ref +FROM pg_stat_statements +WHERE query LIKE '%STMTTS%'; + total | minmax_plan_zero | minmax_exec_zero | minmax_stats_since_after_ref | stats_since_after_ref +---+--+--+--+--- + 2 |0 |0 |0 | 0 +(1 row) + +-- Perform single min/max reset +SELECT
Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints
On Thu, Mar 24, 2022 at 12:12 PM Dilip Kumar wrote: > Thanks, I have gone through your changes in comments and docs and those LGTM. It looks like this patch will need to be updated for Alvaro's commit 49d9cfc68bf4e0d32a948fe72d5a0ef7f464944e. The newly added test 029_replay_tsp_drops.pl fails with this patch applied. The standby log shows: 2022-03-25 10:00:10.022 EDT [38209] LOG: entering standby mode 2022-03-25 10:00:10.024 EDT [38209] LOG: redo starts at 0/328 2022-03-25 10:00:10.062 EDT [38209] FATAL: could not create directory "pg_tblspc/16385/PG_15_202203241/16390": No such file or directory 2022-03-25 10:00:10.062 EDT [38209] CONTEXT: WAL redo at 0/43EBD88 for Database/CREATE_WAL_LOG: create dir 16385/16390 On a quick look, I'm guessing that XLOG_DBASE_CREATE_WAL_LOG will need to mirror some of the logic that was added to the replay code for the existing strategy, but I haven't figured out the details. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add LZ4 compression in pg_dump
On Fri, Mar 25, 2022 at 01:20:47AM -0400, Greg Stark wrote: > It seems development on this has stalled. If there's no further work > happening I guess I'll mark the patch returned with feedback. Feel > free to resubmit it to the next CF when there's progress. Since it's a reasonably large patch (and one that I had myself started before) and it's only been 20some days since (minor) review comments, and since the focus right now is on committing features, and not reviewing new patches, and this patch is new one month ago, and its 0002 not intended for pg15, therefor I'm moving it to the next CF, where I hope to work with its authors to progress it. -- Justin
Re: Corruption during WAL replay
On Fri, Mar 25, 2022 at 2:07 AM Andres Freund wrote: > We really ought to find a way to get to wider checksums :/ Eh, let's just use longer names for the buildfarm animals and call it good. :-) -- Robert Haas EDB: http://www.enterprisedb.com
Re: Corruption during WAL replay
Robert Haas writes: > On Fri, Mar 25, 2022 at 9:49 AM Tom Lane wrote: >> That'll just reduce the probability of failure, not eliminate it. > I mean, if the expected time to the first failure on even 1 machine > exceeds the time until the heat death of the universe by 10 orders of > magnitude, it's probably good enough. Adding another 16 bits won't get you to that, sadly. Yeah, it *might* extend the MTTF to more than the project's likely lifespan, but that doesn't mean we couldn't get unlucky next week. regards, tom lane
Re: pg_relation_size on partitioned table
On Fri, 25 Mar 2022 at 21:21, Bharath Rupireddy wrote: > On Fri, Mar 25, 2022 at 6:23 PM Japin Li wrote: >> >> Hi, hackers >> >> When I try to get total size of partition tables though partitioned table >> name using pg_relation_size(), it always returns zero. I can use the >> following SQL to get total size of partition tables, however, it is a bit >> complex. >> >> SELECT >> pg_size_pretty(sum(pg_relation_size(i.inhrelid))) >> FROM >> pg_class c JOIN pg_inherits i ON c.oid = i.inhparent >> WHERE >> relname = 'parent'; >> >> Could we provide a function to get the total size of the partition table >> though the partitioned table name? Maybe we can extend >> the pg_relation_size() to get the total size of partition tables through >> the partitioned table name. > > If we want to have it in the core, why can't it just be a function (in > system_functions.sql) something like below? Not everyone, would know > how to get partition relation size, especially whey they are not using > psql, they can't use the short forms that it provides. > > CREATE OR REPLACE FUNCTION pg_partition_relation_size(regclass) > RETURNS bigint > LANGUAGE sql > PARALLEL SAFE STRICT COST 1 > BEGIN ATOMIC > SELECT > pg_size_pretty(sum(pg_relation_size(i.inhrelid))) > FROM > pg_class c JOIN pg_inherits i ON c.oid = i.inhparent > WHERE > relname = '$1'; > END; > Yeah, it's a good idea! How about add a fork parameter? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: [PATCH] pg_statio_all_tables: several rows per table due to invalid TOAST index
Andrei Zubkov writes: > Thank you for your attention and for the problem resolution. However > I'm worry a little about possible performance issues related to > monitoring solutions performing regular sampling of statistic views to > find out the most intensive objects in a database. There's no actual evidence that the new formulation is meaningfully worse for such cases. Sure, it's probably *somewhat* less efficient, but that could easily be swamped by pgstat or other costs. I wouldn't care to worry about this unless some evidence is presented that we've created a big problem. regards, tom lane
Re: Corruption during WAL replay
On Fri, Mar 25, 2022 at 9:49 AM Tom Lane wrote: > That'll just reduce the probability of failure, not eliminate it. I mean, if the expected time to the first failure on even 1 machine exceeds the time until the heat death of the universe by 10 orders of magnitude, it's probably good enough. -- Robert Haas EDB: http://www.enterprisedb.com
Re: identifying unrecognized node type errors
Ashutosh Bapat writes: > All these functions are too low level to be helpful to know. Knowing > the caller might actually give a hint as to where the unknown node > originated from. We may get that from the stack trace if that's > available. But if we could annotate the error with error_context that > will be super helpful. Is it really that interesting? If function X lacks coverage for node type Y, then X is what needs to be fixed. The exact call chain for any particular failure seems of only marginal interest, certainly not enough to be building vast new infrastructure for. regards, tom lane
Re: Corruption during WAL replay
Andres Freund writes: > On 2022-03-25 01:38:45 -0400, Tom Lane wrote: >> AFAICS, this strategy of whacking a predetermined chunk of the page with >> a predetermined value is going to fail 1-out-of-64K times. > Yea. I suspect that the way the modifications and checksumming are done are > actually higher chance than 1/64k. But even it actually is 1/64k, it's not > great to wait for (#animals * #catalog-changes) to approach a decent > percentage of 1/64k. Exactly. > I'm was curious whether there have been similar issues in the past. Querying > the buildfarm logs suggests not, at least not in the pg_checksums test. That test has only been there since 2018 (b34e84f16). We've probably accumulated a couple hundred initial-catalog-contents changes since then, so maybe this failure arrived right on schedule :-(. > We really ought to find a way to get to wider checksums :/ That'll just reduce the probability of failure, not eliminate it. regards, tom lane
Re: pg_relation_size on partitioned table
On Fri, 25 Mar 2022 at 20:59, Alvaro Herrera wrote:
> On 2022-Mar-25, Japin Li wrote:
>
>> Could we provide a function to get the total size of the partition table
>> though the partitioned table name? Maybe we can extend
>> the pg_relation_size() to get the total size of partition tables through
>> the partitioned table name.
>
> Does \dP+ do what you need?
Thanks for your quick response!
I find the \dP+ use the following SQL:
SELECT n.nspname as "Schema",
c.relname as "Name",
pg_catalog.pg_get_userbyid(c.relowner) as "Owner",
CASE c.relkind WHEN 'p' THEN 'partitioned table' WHEN 'I' THEN
'partitioned index' END as "Type",
inh.inhparent::pg_catalog.regclass as "Parent name",
c2.oid::pg_catalog.regclass as "Table",
s.tps as "Total size",
pg_catalog.obj_description(c.oid, 'pg_class') as "Description"
FROM pg_catalog.pg_class c
LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace
LEFT JOIN pg_catalog.pg_index i ON i.indexrelid = c.oid
LEFT JOIN pg_catalog.pg_class c2 ON i.indrelid = c2.oid
LEFT JOIN pg_catalog.pg_inherits inh ON c.oid = inh.inhrelid,
LATERAL (SELECT pg_catalog.pg_size_pretty(sum(
CASE WHEN ppt.isleaf AND ppt.level = 1
THEN pg_catalog.pg_table_size(ppt.relid) ELSE 0 END))
AS dps,
pg_catalog.pg_size_pretty(sum(pg_catalog.pg_table_size(ppt.relid))) AS tps
FROM pg_catalog.pg_partition_tree(c.oid) ppt) s
WHERE c.relkind IN ('p','I','')
AND c.relname OPERATOR(pg_catalog.~) '^(parent)$' COLLATE
pg_catalog.default
AND pg_catalog.pg_table_is_visible(c.oid)
ORDER BY "Schema", "Type" DESC, "Parent name" NULLS FIRST, "Name";
pg_table_size() includes "main", "vm", "fsm", "init" and "toast", however,
I only care about the "main" fork.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: Probable memory leak with ECPG and AIX
Guillaume Lelarge writes: > Did this get anywhere? Is there something we could do to make this move > forward? No. Write a patch? regards, tom lane
Re: automatically generating node support functions
Peter Eisentraut writes: > On 24.03.22 22:57, David Rowley wrote: >> Also, I'm quite keen to see this work make it into v15. Do you think >> you'll get time to do that? Thanks for working on it. > My thinking right now is to wait for the PG16 branch to open and then > consider putting it in early. +1. However, as noted by David (and I think I made similar points awhile ago), the patch could still use a lot of mop-up work. It'd be prudent to continue working on it so it will actually be ready to go when the branch is made. regards, tom lane
Re: fixing a few backup compression goofs
Hi, The changes look good to me. Thanks, Dipesh
Re: pg_relation_size on partitioned table
On 2022-Mar-25, Japin Li wrote: > Could we provide a function to get the total size of the partition table > though the partitioned table name? Maybe we can extend > the pg_relation_size() to get the total size of partition tables through > the partitioned table name. Does \dP+ do what you need? -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "La espina, desde que nace, ya pincha" (Proverbio africano)
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Fri, Mar 25, 2022 at 01:05:49AM -0400, Greg Stark wrote: > This patch is marked "waiting on author" in the CF. However the most > recent emails have patches and it's not clear to me what's left from > previous reviews that might not be addressed yet. Should this patch be > marked "Needs Review"? > > Anastasia and Alexander are marked as reviewers. Are you still able to > review it or are there still pending issues that need to be resolved > from previous reviews? I still haven't responded to Alexander's feedback, so I need to do that. (Sorry). However, since the patch attracted no attention for 50 some weeks last year, so now is a weird time to shift attention to it. As such, I will move it to the next CF. https://www.postgresql.org/message-id/flat/20210226182019.gu20...@telsasoft.com#da169a0a518bf8121604437d9ab053b3 -- Justin
