Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
On Fri, Jan 3, 2020 at 10:39 PM Tom Lane wrote: > Hmm. Usually this sort of software gets more weird in newer > versions, not less so ;-). Still, it's a starting point. In case I was unclear: I meant to suggest that this may have something to do with Ubuntu having patched the Debian package for who-knows-what reason. This is indicated by the fact that the Version string is "6.1-1ubuntu1.18.04", as opposed to a Debian style Version without the "ubuntu" (I believe that that's the convention they follow). -- Peter Geoghegan
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Peter Geoghegan writes: > On Fri, Jan 3, 2020 at 10:39 PM Tom Lane wrote: >> Attached is a blind attempt to fix this by allowing escape >> sequence(s) instead of spaces between the words. Does this >> work for you? > I'm afraid not; no apparent change. No change in the "Actual output > was" line, either. Meh. I must be too tired to get the regexp syntax right. Will try tomorrow. regards, tom lane
Re: WIP/PoC for parallel backup
On Thu, Dec 19, 2019 at 10:47 PM Robert Haas wrote: > On Thu, Dec 12, 2019 at 10:20 AM Asif Rehman > wrote: > > I have updated the patches (v7 attached) and have taken care of all > issues pointed by Jeevan, additionally > > ran the pgindent on each patch. Furthermore, Command names have been > renamed as suggested and I > > have simplified the SendFiles function. Client can only request the > regular files, any other kind such as > > directories or symlinks will be skipped, the client will be responsible > for taking care of such. > > Hi, > > Patch 0001 of this series conflicts with my recent commit > 303640199d0436c5e7acdf50b837a027b5726594; that commit was actually > inspired by some previous study of 0001. That being said, I think 0001 > has the wrong idea. There's no reason that I can see why it should be > correct to remove the PG_ENSURE_ERROR_CLEANUP calls from > perform_base_backup(). It's true that if we register a long-lived > before_shmem_exit hook, then the backup will get cleaned up even > without the PG_ENSURE_ERROR_CLEANUP block, but there's also the > question of the warning message. I think that our goal should be to > emit the warning message about a backup being stopped too early if the > user uses either pg_start_backup() or the new START_BACKUP command and > does not end the backup with either pg_stop_backup() or the new > STOP_BACKUP command -- but not if a single command that both starts > and ends a backup, like BASE_BACKUP, is interrupted. To accomplish > that goal in the wake of 303640199d0436c5e7acdf50b837a027b5726594, we > need to temporarily register do_pg_abort_backup() as a > before_shmem_exit() handler using PG_ENSURE_ERROR_CLEANUP() during > commands like BASE_BACKUP() -- and for things like pg_start_backup() > or the new START_BACKUP command, we just need to add a single call to > register_persistent_abort_backup_handler(). > > So I think you can drop 0001, and then in the patch that actually > introduces START_BACKUP, add the call to > register_persistent_abort_backup_handler() before calling > do_pg_start_backup(). Also in that patch, also adjust the warning text > that do_pg_abort_backup() emits to be more generic e.g. "aborting > backup due to backend exiting while a non-exclusive backup is in > progress". > > Sure. will do. > 0003 creates three new functions, moving code from > do_pg_start_backup() to a new function collectTablespaces() and from > perform_base_backup() to new functions setup_throttle() and > include_wal_files(). I'm skeptical about all of these changes. One > general nitpick is that the way these function names are capitalized > and punctuated does not seem to have been chosen very consistently; > how about name_like_this() throughout? A bit more substantively: > > - collectTablespaces() is factored out of do_pg_start_backup() so that > it can also be used by SendFileList(), but that means that a client is > going to invoke START_BACKUP, indirectly calling collectTablespaces(), > and then immediately afterward the client is probably going to call > SEND_FILE_LIST, which will again call collectTablespaces(). That does > not appear to be super-great. For one thing, it's duplicate work, > although because SendFileList() is going to pass infotbssize as false, > it's not a lot of duplicated work. I'll remove this duplication by eliminating this call from START_BACKUP and SEND_FILE_LIST functions. More about this is explained later in this email. > Also, what happens if the two calls > to collectTablespaces() return different answers due to concurrent > CREATE/DROP TABLESPACE commands? Maybe it would all work out fine, but > it seems like there is at least the possibility of bugs if different > parts of the backup have different notions of what tablespaces exist. > The concurrent CREATE/DROP TABLESPACE commands, it can happen and will be resolved by the WAL files collected for the backup. I don't think we can do anything when objects are created or dropped in-between start and stop backup. BASE_BACKUPalso relies on the WAL files to handle such a scenario and does not error out when some relation files go away. > > - setup_throttle() is factored out of perform_base_backup() so that it > can be called in StartBackup() and StopBackup() and SendFiles(). This > seems extremely odd. Why does it make any sense to give the user an > option to activate throttling when *ending* a backup? Why does it make > sense to give the user a chance to enable throttling *both* at the > startup of a backup *and also* for each individual file. If we're > going to support throttling here, it seems like it should be either a > backup-level property or a file-level property, not both. > It's a file-level property only. Throttle functionality relies on global variables. StartBackup() and StopBackup() are calling setup_throttle function to disable the throttling. I should have been more explicit here by using -1 to setup_throttle, Illustrating that throttling is
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
On Fri, Jan 3, 2020 at 10:39 PM Tom Lane wrote: > Attached is a blind attempt to fix this by allowing escape > sequence(s) instead of spaces between the words. Does this > work for you? I'm afraid not; no apparent change. No change in the "Actual output was" line, either. -- Peter Geoghegan
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Peter Geoghegan writes:
> On Fri, Jan 3, 2020 at 9:30 PM Tom Lane wrote:
>> BTW, it seems somewhat likely that this is less about libreadline
>> than about its dependency libtinfo. On my machine that's from
>> ii libtinfo6:amd64 6.1+20181013-2+deb10u2
>> amd64shared low-level terminfo library for terminal handling
> This seems promising. By following the same ldd + dpkg -S workflow as
> before, I can see that my libtinfo is "libtinfo5:amd64".
Hmm. Usually this sort of software gets more weird in newer
versions, not less so ;-). Still, it's a starting point.
Attached is a blind attempt to fix this by allowing escape
sequence(s) instead of spaces between the words. Does this
work for you?
regards, tom lane
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index 96221f8..7f1797c 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -38,6 +38,12 @@ $node->safe_psql('postgres',
my $historyfile = "${TestLib::log_path}/010_psql_history.txt";
$ENV{PSQL_HISTORY} = $historyfile;
+# Ensure that readline/libedit puts out xterm escapes, not something else.
+$ENV{TERM} = 'xterm';
+
+# regexp to match one xterm escape sequence (CSI style only, for now)
+my $escseq = "(\e\\[[0-9;]*[A-Za-z])";
+
# fire up an interactive psql session
my $in = '';
my $out = '';
@@ -101,8 +107,12 @@ check_completion(
"select \\* from my\a?tab",
"complete my to mytab when there are multiple choices");
-# some versions of readline/libedit require two tabs here, some only need one
-check_completion("\t\t", "mytab123 +mytab246",
+# some versions of readline/libedit require two tabs here, some only need one.
+# also, some might issue escape sequences to reposition the cursor, instead
+# of just printing some spaces.
+check_completion(
+ "\t\t",
+ "mytab$escseq*123( +|$escseq+)mytab$escseq*246",
"offer multiple table choices");
check_completion("2\t", "246 ",
Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions
On Sat, Jan 4, 2020 at 10:00 AM Amit Kapila wrote:
>
> On Sun, Dec 29, 2019 at 1:34 PM Dilip Kumar wrote:
> > On Sat, Dec 28, 2019 at 9:33 PM Tomas Vondra
> > wrote:
> > +static void
> > +set_schema_sent_in_streamed_txn(RelationSyncEntry *entry, TransactionId
> > xid)
> > +{
> > + MemoryContext oldctx;
> > +
> > + oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> > +
> > + entry->streamed_txns = lappend_int(entry->streamed_txns, xid);
> > +
> > + MemoryContextSwitchTo(oldctx);
> > +}
> > I was looking into the schema tracking solution and I have one
> > question, Shouldn't we remove the topxid from the list if the
> > (sub)transaction is aborted? because once it is aborted we need to
> > resent the schema.
> >
>
> I think you are right because, at abort, the subscriber would remove
> the changes (for a subtransaction) including the schema changes sent
> and then it won't be able to understand the subsequent changes sent by
> the publisher. Won't we need to remove xid from the list at commit
> time as well, otherwise, the list will keep on growing.
Yes, we need to remove the xid from the list at the time of commit as well.
One more
> thing, we need to search the list of all the relations in the local
> map to find xid being aborted/committed, right? If so, won't it be
> costly doing at each transaction abort/commit?
Yeah, if multiple concurrent transactions operate on the common
relations then the list can grow longer. I am not sure how many
concurrent large transactions are possible maybe it won't be huge that
searching will be very costly. Otherwise, we can maintain the sorted
array of the xids and do a binary search or we can maintain hash?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
Hello Sawada and all I would like to elaborate more on Sehrope and Sawada's discussion on passing NULL IV in "pg_cipher_encrypt/decrypt" functions during kmgr_wrap_key and kmgr_unwrap_key routines in kmgr_utils.c. Openssl implements key wrap according to RFC3394 as Sawada mentioned and passing NULL will make openssl to use default IV, which equals to A6A6A6A6A6A6A6A6. I have confirmed this on my side; A key wrapped with "NULL" IV can be unwrapped successfully with IV=A6A6A6A6A6A6A6A6, and unwrap will fail if IV is set to anything else other than NULL or A6A6A6A6A6A6A6A6. I would like to provide some comments on the encryption and decryption routines provided by cipher_openssl.c in which cipher.c and kmgr_utils.c are using. I see that "ossl_cipher_encrypt" calls "EVP_EncryptInit_ex" and "EVP_EncryptUpdate" only to complete the encryption. Same thing applies to decryption routines. According to my past experience with openssl and the usages online, it is highly recommended to use "init-update-final" cycle to complete the encryption and I see that the "final" part (EVP_EncryptFinal) is missing. This call will properly handle the last block of data especially when padding is taken into account. The functions still works now because the input is encryption key and its size is multiple of each cipher block and no padding is used. I think it will be safer to use the proper "init-update-final" cycle for encryption/decryption According to openssl EVP documentation, "EVP_EncryptUpdate" can be called multiple times at different offset to the input data to be encrypted. I see that "pg_cipher_encrypt" only calls "EVP_EncryptUpdate" once, which makes me assume that the application should invoke "pg_cipher_encrypt" multiple times until the entire data block is encrypted? I am asking because if we were to use "EVP_EncryptFinal" to complete the encryption cycle, then it is better to let "pg_cipher_encrypt" to figure out how many times "EVP_EncryptUpdate" should be called and finalize it with "EVP_EncryptFinal" at last block. Lastly, I think we are missing a cleanup routine that calls "EVP_CIPHER_CTX_free()" to free up the EVP_CIPHER_CTX when encryption is done. Thank you Cary Huang HighGo Software Canada
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
On Fri, Jan 3, 2020 at 9:30 PM Tom Lane wrote: > BTW, it seems somewhat likely that this is less about libreadline > than about its dependency libtinfo. On my machine that's from > > ii libtinfo6:amd64 6.1+20181013-2+deb10u2 > amd64shared low-level terminfo library for terminal handling This seems promising. By following the same ldd + dpkg -S workflow as before, I can see that my libtinfo is "libtinfo5:amd64". This libtinfo appears to be a Ubuntu-specific package: $ dpkg -l libtinfo5:amd64 Desired=Unknown/Install/Remove/Purge/Hold | Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend |/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad) ||/ Name Version Architecture Description +++--===-===-== ii libtinfo5:amd64 6.1-1ubuntu1.18.04 amd64 shared low-level terminfo library for terminal handling -- Peter Geoghegan
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Peter Geoghegan writes:
> On Fri, Jan 3, 2020 at 7:06 PM Peter Geoghegan wrote:
>> No. Also tried setting PG_COLOR="off" and CLICOLOR=0 -- that also
>> didn't help. (This was based on possibly-relevant vars that "env"
>> showed were set).
Yeah, that's not terribly surprising, because if I'm reading those
escape sequences correctly they're not about color. They seem to be
just cursor movement and line clearing, according to [1].
What I'm mystified by is why your copy of libreadline is choosing to
do that, rather than just space over to where the word should be printed
which is what every other copy seems to be doing. I have a fresh new
Debian installation at hand, with
$ dpkg -l | grep readline
ii libreadline-dev:amd64 7.0-5
amd64GNU readline and history libraries, development files
ii libreadline5:amd645.2+dfsg-3+b13
amd64GNU readline and history libraries, run-time libraries
ii libreadline7:amd647.0-5
amd64GNU readline and history libraries, run-time libraries
ii readline-common 7.0-5
all GNU readline and history libraries, common files
and I'm not seeing the failure on it, either with TERM=xterm
or with TERM=xterm-256color. So what's the missing ingredient?
> Removing the single check_completion() test from 010_tab_completion.pl
> that actually fails on my system ("offer multiple table choices")
> fixes the problem for me -- everything else passes.
> I suppose that this means that the problem is in "offer multiple table
> choices" specifically.
I'd hate to conclude that we can't test any completion behavior that
involves offering a list.
If we can't coerce libreadline into being less avant-garde in its
screen management, I suppose we could write a regex to recognize
xterm escape sequences and ignore those. But I'd be happier about
this if I could reproduce the behavior. I don't like the feeling
that there's something going on here that I don't understand.
BTW, it seems somewhat likely that this is less about libreadline
than about its dependency libtinfo. On my machine that's from
ii libtinfo6:amd64 6.1+20181013-2+deb10u2
amd64shared low-level terminfo library for terminal handling
what about yours?
regards, tom lane
[1] https://www.xfree86.org/current/ctlseqs.html
Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions
On Sun, Dec 29, 2019 at 1:34 PM Dilip Kumar wrote:
> On Sat, Dec 28, 2019 at 9:33 PM Tomas Vondra
> wrote:
> >
> >
> > Yeah, the "is_schema_sent" flag in ReorderBufferTXN does not work - it
> > needs to be in the RelationSyncEntry. In fact, I already have code for
> > that in my private repository - I thought the patches I sent here do
> > include this, but apparently I forgot to include this bit :-(
> >
> > Attached is a rebased patch series, fixing this. It's essentially v2
> > with a couple of patches (0003, 0008, 0009 and 0012) replacing the
> > is_schema_sent with correct handling.
> >
> >
> > 0003 - removes an is_schema_sent reference added prematurely (it's added
> > by a later patch, causing compile failure)
> >
> > 0008 - adds the is_schema_sent back (essentially reverting 0003)
> >
> > 0009 - removes is_schema_sent entirely
> >
> > 0012 - adds the correct handling of schema flags in pgoutput
> >
Thanks for splitting the changes. They are quite clear.
> >
> > I don't know what other changes you've made since v2, so this way it
> > should be possible to just take 0003, 0008, 0009 and 0012 and slip them
> > in with minimal hassle.
> >
> > FWIW thanks to everyone (and Amit and Dilip in particular) working on
> > this patch series. There's been a lot of great reviews and improvements
> > since I abandoned this thread for a while. I expect to be able to spend
> > more time working on this in January.
> >
> +static void
> +set_schema_sent_in_streamed_txn(RelationSyncEntry *entry, TransactionId xid)
> +{
> + MemoryContext oldctx;
> +
> + oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> +
> + entry->streamed_txns = lappend_int(entry->streamed_txns, xid);
> +
> + MemoryContextSwitchTo(oldctx);
> +}
> I was looking into the schema tracking solution and I have one
> question, Shouldn't we remove the topxid from the list if the
> (sub)transaction is aborted? because once it is aborted we need to
> resent the schema.
>
I think you are right because, at abort, the subscriber would remove
the changes (for a subtransaction) including the schema changes sent
and then it won't be able to understand the subsequent changes sent by
the publisher. Won't we need to remove xid from the list at commit
time as well, otherwise, the list will keep on growing. One more
thing, we need to search the list of all the relations in the local
map to find xid being aborted/committed, right? If so, won't it be
costly doing at each transaction abort/commit?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
On Fri, Jan 3, 2020 at 7:06 PM Peter Geoghegan wrote:
> No. Also tried setting PG_COLOR="off" and CLICOLOR=0 -- that also
> didn't help. (This was based on possibly-relevant vars that "env"
> showed were set).
Removing the single check_completion() test from 010_tab_completion.pl
that actually fails on my system ("offer multiple table choices")
fixes the problem for me -- everything else passes.
I suppose that this means that the problem is in "offer multiple table
choices" specifically.
--
Peter Geoghegan
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
On Fri, Jan 3, 2020 at 6:51 PM Tom Lane wrote: > Hmm. If you set it to plain "xterm", does the test pass? No. Also tried setting PG_COLOR="off" and CLICOLOR=0 -- that also didn't help. (This was based on possibly-relevant vars that "env" showed were set). -- Peter Geoghegan
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Peter Geoghegan writes: >> I'm curious also what is your prevailing setting >> of TERM? > I use zsh, with a fairly customized setup. $TERM is "xterm-256color" > in the affected shell. (I have a feeling that this has something to do > with my amazing technicolor terminal.) Hmm. If you set it to plain "xterm", does the test pass? regards, tom lane
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
On Fri, Jan 3, 2020 at 6:16 PM Tom Lane wrote: > Huh. What readline or libedit version are you using, on what > platform? Ubuntu 18.04. I used ldd to verify that psql links to the system libreadline, which is libreadline7:amd64 -- that's what Debian packages as "7.0-3". > I'm curious also what is your prevailing setting > of TERM? I use zsh, with a fairly customized setup. $TERM is "xterm-256color" in the affected shell. (I have a feeling that this has something to do with my amazing technicolor terminal.) -- Peter Geoghegan
Re: Greatest Common Divisor
On 03/01/2020 20:14, Fabien COELHO wrote:
>
> Bonsoir Vik,
>
> +int4gcd_internal(int32 arg1, int32 arg2)
> +{
> + int32 swap;
> +
> + /*
> + * Put the greater value in arg1.
> + * This would happen automatically in the loop below, but
> avoids an
> + * expensive modulo simulation on some architectures.
> + */
> + if (arg1 < arg2)
> + {
> + swap = arg1;
> + arg1 = arg2;
> + arg2 = swap;
> + }
>
>
> The point of swapping is to a void possibly expensive modulo, but this
> should be done on absolute values, otherwise it may not achieve its
> purpose as stated by the comment?
Here is an updated patch fixing that.
--
Vik Fearing
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 57a1539506..e2b7d6d240 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -870,6 +870,32 @@
-43
+
+
+
+ gcd
+
+gcd(a, b)
+
+ (same as argument types)
+ greatest common divisor
+ gcd(1071, 462)
+ 21
+
+
+
+
+
+ lcm
+
+lcm(a, b)
+
+ (same as argument types)
+ least common multiple
+ lcm(1071, 462)
+ 23562
+
+
diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index 583ce71e66..611207e642 100644
--- a/src/backend/utils/adt/int.c
+++ b/src/backend/utils/adt/int.c
@@ -1196,6 +1196,212 @@ int2abs(PG_FUNCTION_ARGS)
PG_RETURN_INT16(result);
}
+/*
+ * Greatest Common Divisor
+ *
+ * Special cases:
+ * - gcd(x, 0) = x; because 0 is divisible by anything
+ * - gcd(0, 0) = 0; complies with the previous definition and is a
+ *common convention
+ *
+ * The following cases involving INT_MIN have two possible results. They could
+ * return INT_MIN because INT_MIN is a valid divisor of INT_MIN, or they could
+ * throw an exception because the result is negative.
+ * The consensus is to throw an exception.
+ *
+ * - gcd(INT_MIN, 0)
+ * - gcd(INT_MIN, INT_MIN)
+ *
+ * Any other value with INT_MIN will be a positive value representable within
+ * the data type.
+ */
+static int32
+int4gcd_internal(int32 arg1, int32 arg2)
+{
+ int32 swap;
+ int32 a1, a2;
+
+ /*
+ * Put the greater absolute value in arg1.
+ *
+ * This would happen automatically in the loop below, but avoids an
+ * expensive modulo simulation on some architectures.
+ *
+ * We do this in negative space in order to handle INT_MIN.
+ */
+ a1 = (arg1 < 0) ? arg1 : -arg1;
+ a2 = (arg2 < 0) ? arg2 : -arg2;
+ if (a1 > a2)
+ {
+ swap = arg1;
+ arg1 = arg2;
+ arg2 = swap;
+ }
+
+ /* Special care needs to be taken with INT_MIN. See comments above. */
+ if (arg1 == PG_INT32_MIN)
+ {
+ if (arg2 == 0 || arg2 == PG_INT32_MIN)
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("integer out of range")));
+ /*
+ * Making arg2 positive avoids the INT_MIN % -1 issue described in
+ * int4mod.
+ */
+ arg2 = -arg2;
+ }
+
+ /* Find GCD using the basic Euclidean algorithm */
+ while (arg2 != 0)
+ {
+ swap = arg2;
+ arg2 = arg1 % arg2;
+ arg1 = swap;
+ }
+
+ /*
+ * Make sure the result is positive. (We know we don't have INT_MIN
+ * anymore).
+ */
+ if (arg1 < 0)
+ arg1 = -arg1;
+
+ return arg1;
+}
+
+static int16
+int2gcd_internal(int16 arg1, int16 arg2)
+{
+ /* See int4gcd_internal for commented version. */
+ int16 swap;
+ int16 a1, a2;
+
+ a1 = (arg1 < 0) ? arg1 : -arg1;
+ a2 = (arg2 < 0) ? arg2 : -arg2;
+ if (a1 > a2)
+ {
+ swap = arg1;
+ arg1 = arg2;
+ arg2 = swap;
+ }
+
+ if (arg1 == PG_INT16_MIN)
+ {
+ if (arg2 == 0 || arg2 == PG_INT16_MIN)
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("smallint out of range")));
+ arg2 = -arg2;
+ }
+
+ while (arg2 != 0)
+ {
+ swap = arg2;
+ arg2 = arg1 % arg2;
+ arg1 = swap;
+ }
+
+ if (arg1 < 0)
+ arg1 = -arg1;
+
+ return arg1;
+}
+
+Datum
+int4gcd(PG_FUNCTION_ARGS)
+{
+ int32 arg1 = PG_GETARG_INT32(0);
+ int32 arg2 = PG_GETARG_INT32(1);
+ int32 result;
+
+ result = int4gcd_internal(arg1, arg2);
+
+ PG_RETURN_INT32(result);
+}
+
+Datum
+int2gcd(PG_FUNCTION_ARGS)
+{
+ int16 arg1 = PG_GETARG_INT16(0);
+ int16 arg2 = PG_GETARG_INT16(1);
+ int16 result;
+
+ result = int2gcd_internal(arg1, arg2);
+
+ PG_RETURN_INT16(result);
+}
+
+/*
+ * Least Common Multiple
+ *
+ * Both arguments must be positive. If either argument is 0 we can just return
+ * 0 without further calculation. This has the nice side effect of avoiding a
+ * division by zero for lcm(0, 0) since we use gcd in the formula. Also, if
+ * both arguments are the same, we can just return it without further
+ * calculation.
+ */
+
+Datum
+int4lcm(PG_FUNCTION_ARGS)
+{
+ int32 arg1 = PG_GETARG_INT32(0);
+ int32 arg2 = PG_GETARG_INT32(1);
+ int32 gcd;
+ int32 result;
+
+ if (arg1 < 0 || arg2 < 0)
+
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Peter Geoghegan writes: > Not sure if the specifics matter, but FWIW "make check-world" ended > with the following failure just now: > # Failed test 'offer multiple table choices' > # at t/010_tab_completion.pl line 105. > # Actual output was "\r\n\e[01;35mmytab\e[0m\e[K123\e[0m\e[K > \e[01;35mmytab\e[0m\e[K246\e[0m\e[K \r\npostgres=# select * from > mytab\r\n\e[01;35mmytab\e[0m\e[K123\e[0m\e[K > \e[01;35mmytab\e[0m\e[K246\e[0m\e[K \r\npostgres=# select * from > mytab" Huh. What readline or libedit version are you using, on what platform? I'm curious also what is your prevailing setting of TERM? (I've been wondering if the test doesn't need to force that to something standard. The buildfarm hasn't shown any signs of needing that, but manual invocations might be a different story.) regards, tom lane
Re: Assigning ROW variable having NULL value to RECORD type variable doesn't give any structure to the RECORD variable.
On Sat, Jan 4, 2020 at 2:09 AM Tom Lane wrote: > > Robert Haas writes: > > On Wed, Jan 1, 2020 at 10:50 AM Ashutosh Sharma > > wrote: > >> I know this is expected to happen considering the changes done in > >> above commit because from this commit onwards, NULL value assigned to > >> any row variable represents a true NULL composite value before this > >> commit it used to be a tuple with each column having null value in it. > >> But, the point is, even if the row variable is having a NULL value it > >> still has a structure associated with it. Shouldn't that structure be > >> transferred to RECORD variable when it is assigned with a ROW type > >> variable ? Can we consider this behaviour change as a side effect of > >> the improvement done in the RECORD type of variable? > > > I'm not an expert on this topic. However, I *think* that you're trying > > to distinguish between two things that are actually the same. If it's > > a "true NULL," it has no structure; it's just NULL. If it has a > > structure, then it's really a composite value with a NULL in each > > defined column, i.e. (NULL, NULL, NULL, ...) for some row type rather > > than just NULL. > > Yeah. In general, we can't do this, because a null value of type > RECORD simply hasn't got any information about what specific rowtype > might be involved. In the case where the null is of a named composite > type, rather than RECORD, we could choose to act differently ... but > I'm not really sure that such a change would be an improvement and not > just a decrease in consistency. > > In any case, plpgsql's prior behavior was an implementation artifact > with very little to recommend it. As a concrete example, consider > > create table t1(a int, b text); > > do $$ > declare x t1; r record; > begin > x := null; > r := x; > raise notice 'r.a = %', r.a; > end $$; > > do $$ > declare r record; > begin > r := null::t1; > raise notice 'r.a = %', r.a; > end $$; > > I assert that in any sanely-defined semantics, these two examples > should give the same result. In v11 and up, they both give > 'record "r" is not assigned yet' ... but in prior versions, they > gave different results. I do not want to go back to that. > > On the other hand, we now have > > do $$ > declare x t1; r record; > begin > x := null; > r := x; > raise notice 'x.a = %', x.a; > raise notice 'r.a = %', r.a; > end $$; > > which gives > > NOTICE: x.a = > ERROR: record "r" is not assigned yet > > which is certainly also inconsistent. The variable declared as > being type t1 behaves, for this purpose, as if it contained > "row(null,null)" not just a simple null. But if you print it, > or assign it to something else as a whole, you'll find it just > contains a simple null. One way to see that these are different > states is to do > > do $$ declare x t1; begin x := null; raise notice 'x = %', x; end$$; > NOTICE: x = > > versus > > do $$ declare x t1; begin x := row(null,null); raise notice 'x = %', x; end$$; > NOTICE: x = (,) > > And, if you assign a row of nulls to a record-type variable, that works: > > do $$ > declare x t1; r record; > begin > x := row(null,null); > r := x; > raise notice 'x.a = %', x.a; > raise notice 'r.a = %', r.a; > end $$; > > which gives > > NOTICE: x.a = > NOTICE: r.a = > > If we were to change this behavior, I think it would be tantamount > to sometimes expanding a simple null to a row of nulls, and I'm > not sure that's a great idea. > > The SQL standard is confusing in this respect, because it seems > that at least the "x IS [NOT] NULL" construct is defined to > consider both a "simple NULL" and ROW(NULL,NULL,...) as "null". > But we've concluded that other parts of the spec do allow for > a distinction (I'm too lazy to search the archives for relevant > discussions, but there have been some). The two things are > definitely different implementation-wise, so it would be hard > to hide the difference completely. > > Another fun fact is that right now, assignment of any null value > to a composite plpgsql variable works the same: you can assign a simple > null of some other composite type, or even a scalar null, and behold you > get a null composite value without any error. That's because > exec_assign_value's DTYPE_REC case pays no attention to the declared > type of the source value once it's found to be null. Thus > > do $$ declare x t1; begin x := 42; raise notice 'x = %', x; end$$; > ERROR: cannot assign non-composite value to a record variable > > do $$ declare x t1; begin x := null::int; raise notice 'x = %', x; end$$; > NOTICE: x = > > That's pretty bizarre, and I don't think I'd agree with adopting those > semantics if we were in a green field. But if we start paying attention > to the specific type of a null source value, I bet we're going to break > some code that works today. > > Anyway, maybe this area could be improved, but I'm not fully convinced. > I definitely do not subscribe to the theory that we need
Re: [HACKERS] Block level parallel vacuum
On Fri, Jan 3, 2020 at 10:15 PM Robert Haas wrote: > > On Sun, Dec 29, 2019 at 4:23 PM Tomas Vondra > wrote: > > IMO there's not much reason for the leader not to participate. For > > regular queries the leader may be doing useful stuff (essentially > > running the non-parallel part of the query) but AFAIK for VAUCUM that's > > not the case and the worker is not doing anything. > > I agree, and said the same thing in > http://postgr.es/m/CA+Tgmob7JLrngeHz6i60_TqdvE1YBcvGYVoEQ6_xvP=vn7d...@mail.gmail.com > > I really don't know why we have that code. > We have removed that code from the main patch. It is in a separate patch and used mainly for development testing where we want to debug/test the worker code. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
On Fri, Jan 3, 2020 at 9:59 AM Robert Haas wrote: > I take no position on whether Debian is correct in its assessment of > such things, but I reiterate my previous opposition to breaking it > just because we don't agree with it, or because Tom specifically > doesn't. It's too mainstream a platform to arbitrarily break. And it > will probably just have the effect of increasing the number of patches > they're carrying against our sources, which will not make things > better for anybody. Even with commit 56a3921a, "make check-world" is broken on my Ubuntu 18.04 workstation. This is now adversely impacting my work, so I hope it can be resolved soon. Not sure if the specifics matter, but FWIW "make check-world" ended with the following failure just now: make[2]: Entering directory '/code/postgresql/patch/build/src/bin/psql' rm -rf '/code/postgresql/patch/build/src/bin/psql'/tmp_check /bin/mkdir -p '/code/postgresql/patch/build/src/bin/psql'/tmp_check cd /code/postgresql/patch/build/../source/src/bin/psql && TESTDIR='/code/postgresql/patch/build/src/bin/psql' PATH="/code/postgresql/patch/build/tmp_install/code/postgresql/patch/install/bin:$PATH" LD_LIBRARY_PATH="/code/postgresql/patch/build/tmp_install/code/postgresql/patch/install/lib" PGPORT='65432' PG_REGRESS='/code/postgresql/patch/build/src/bin/psql/../../../src/test/regress/pg_regress' REGRESS_SHLIB='/code/postgresql/patch/build/src/test/regress/regress.so' /usr/bin/prove -I /code/postgresql/patch/build/../source/src/test/perl/ -I /code/postgresql/patch/build/../source/src/bin/psql t/*.pl t/010_tab_completion.pl .. 8/? # Failed test 'offer multiple table choices' # at t/010_tab_completion.pl line 105. # Actual output was "\r\n\e[01;35mmytab\e[0m\e[K123\e[0m\e[K \e[01;35mmytab\e[0m\e[K246\e[0m\e[K \r\npostgres=# select * from mytab\r\n\e[01;35mmytab\e[0m\e[K123\e[0m\e[K \e[01;35mmytab\e[0m\e[K246\e[0m\e[K \r\npostgres=# select * from mytab" # # Looks like you failed 1 test of 12. t/010_tab_completion.pl .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/12 subtests Test Summary Report --- t/010_tab_completion.pl (Wstat: 256 Tests: 12 Failed: 1) Failed test: 8 Non-zero exit status: 1 Files=1, Tests=12, 7 wallclock secs ( 0.02 usr 0.00 sys + 0.80 cusr 0.09 csys = 0.91 CPU) Result: FAIL Makefile:87: recipe for target 'check' failed make[2]: *** [check] Error 1 make[2]: Leaving directory '/code/postgresql/patch/build/src/bin/psql' Makefile:41: recipe for target 'check-psql-recurse' failed make[1]: *** [check-psql-recurse] Error 2 make[1]: Leaving directory '/code/postgresql/patch/build/src/bin' GNUmakefile:70: recipe for target 'check-world-src/bin-recurse' failed make: *** [check-world-src/bin-recurse] Error 2 -- Peter Geoghegan
Re: sidewinder has one failure
On Sat, Jan 4, 2020 at 6:19 AM Tom Lane wrote: > > =?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= writes: > > I tried starting it from cron and then I got: > > max_safe_fds = 981, usable_fds = 1000, already_open = 9 > > Oh! There we have it then. > Right. > I wonder if that's a cron bug (neglecting > to close its own FDs before forking children) or intentional (maybe > it uses those FDs to keep tabs on the children?). > So, where do we go from here? Shall we try to identify why cron is keeping extra FDs or we assume that we can't predict how many pre-opened files there will be? In the latter case, we either want to (a) tweak the test to raise the value of max_files_per_process, (b) remove the test entirely. You seem to incline towards (b), but I have a few things to say about that. We have another strange failure due to this test on one of Noah's machine, see my email [1]. I have requested Noah for the stack trace [2]. It is not clear to me whether the committed code has any problem or the test has discovered a different problem in v10 specific to that platform. The same test has passed for v11, v12, and HEAD on the same platform. [1] - https://www.postgresql.org/message-id/CAA4eK1LMDx6vK8Kdw8WUeW1MjToN2xVffL2kvtHvZg17%3DY6QQg%40mail.gmail.com [2] - https://www.postgresql.org/message-id/CAA4eK1LJqMuXoCLuxkTr1HidbR8DkgRrVC7jHWDyXT%3DFD2gt6Q%40mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: sidewinder has one failure
=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= writes: > I tried starting it from cron and then I got: > max_safe_fds = 981, usable_fds = 1000, already_open = 9 Oh! There we have it then. I wonder if that's a cron bug (neglecting to close its own FDs before forking children) or intentional (maybe it uses those FDs to keep tabs on the children?). regards, tom lane
Re: sidewinder has one failure
=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= writes: > On 2020-01-04 01:15, Tom Lane wrote: >> Apparently, in the environment of that TAP test, the server has more >> open FDs at this point than it does when running "normally". I have >> no idea what the additional FDs might be. > Well it's running under cron if that makes a difference and what is the > TAP-test using? perl? Not sure. There's a few things you could do to investigate: * Run the recovery TAP tests. Do you reproduce the buildfarm failure in your hand build? If not, we need to ask what's different. * If you do reproduce it, run those tests at debug2, just to confirm the theory that already_open is higher than normal. (The easy way to make that happen is to add another line to what PostgresNode.pm's init function is adding to postgresql.conf.) * Also, try putting a pg_usleep call just before the error in fd.c, to give yourself enough time to manually point "lsof" at the postmaster and see what all its FDs are. regards, tom lane
Re: sidewinder has one failure
On 2020-01-04 01:21, Mikael Kjellström wrote: Apparently, in the environment of that TAP test, the server has more open FDs at this point than it does when running "normally". I have no idea what the additional FDs might be. Well it's running under cron if that makes a difference and what is the TAP-test using? perl? I tried starting it from cron and then I got: max_safe_fds = 981, usable_fds = 1000, already_open = 9 /Mikael
Re: Greatest Common Divisor
On 04/01/2020 01:26, Vik Fearing wrote: > On 04/01/2020 01:21, Tom Lane wrote: >> Vik Fearing writes: >>> On 03/01/2020 20:14, Fabien COELHO wrote: I'm unsure about gcd(INT_MIN, 0) should error. Possibly 0 would be nicer? >>> What justification for that do you have? >> Zero is the "correct" answer for that, isn't it, independently of overflow >> considerations? > > I would say not. The correct answer is INT_MIN but we've decided a > negative result is not desirable. Wolfram Alpha agrees. https://www.wolframalpha.com/input/?i=gcd%28-9223372036854775808%2C0%29 -- Vik Fearing
Re: Greatest Common Divisor
Vik Fearing writes: > On 04/01/2020 01:21, Tom Lane wrote: >> Zero is the "correct" answer for that, isn't it, independently of overflow >> considerations? > I would say not. Oh, right, I was misremembering the identity gcd(a,0) as being 0 not a. Never mind that then. > The correct answer is INT_MIN but we've decided a > negative result is not desirable. Agreed. On the other hand, we could stave off overflow the same way we discussed for lcm: make it return int8. We're still stuck with the special case for INT64_MIN in gcd64 of course, so maybe that's just inconsistent rather than being worthwhile. [ thinks for a bit... ] In practice, I imagine few people use gcd on negative values, so doing weird things with the datatype choices is probably not better than throwing an error for this case. regards, tom lane
Re: Greatest Common Divisor
On 04/01/2020 01:21, Tom Lane wrote: > Vik Fearing writes: >> On 03/01/2020 20:14, Fabien COELHO wrote: >>> I'm unsure about gcd(INT_MIN, 0) should error. Possibly 0 would be nicer? >> What justification for that do you have? > Zero is the "correct" answer for that, isn't it, independently of overflow > considerations? I would say not. The correct answer is INT_MIN but we've decided a negative result is not desirable. > We should strive to give the correct answer if it's known > and representable, rather than have arbitrary failure conditions. On that we fully agree. > (IOW, we should throw errors only when the *result* is out of range > or undefined, not just because the input is an edge case.) That's what I do with the rest of it. INT_MIN is only an error if the result of the calculation is also INT_MIN. -- Vik Fearing
Re: Greatest Common Divisor
Vik Fearing writes: > On 03/01/2020 20:14, Fabien COELHO wrote: >> I'm unsure about gcd(INT_MIN, 0) should error. Possibly 0 would be nicer? > What justification for that do you have? Zero is the "correct" answer for that, isn't it, independently of overflow considerations? We should strive to give the correct answer if it's known and representable, rather than have arbitrary failure conditions. (IOW, we should throw errors only when the *result* is out of range or undefined, not just because the input is an edge case.) regards, tom lane
Re: Greatest Common Divisor
On 04/01/2020 00:49, Tom Lane wrote: > Vik Fearing writes: >> On 03/01/2020 20:14, Fabien COELHO wrote: >>> The point of swapping is to a void possibly expensive modulo, but this >>> should be done on absolute values, otherwise it may not achieve its >>> purpose as stated by the comment? >> Ah, true. How widespread are these architectures that need this special >> treatment? Is it really worth handling? > On some older RISC architectures, integer division is really slow, like > slower than floating-point. I'm not sure if that's true on any platform > people still care about though. In recent years, CPU architects have been > able to throw all the transistors they needed at such problems. On a > machine with single-cycle divide, it's likely that the extra > compare-and-branch is a net loss. OK. > Might be worth checking it on ARM in particular, as being a RISC > architecture that's still popular. I don't know how I would check this. > Also, if we end up having a "numeric" implementation, it absolutely is > worth it for that, because there is nothing cheap about numeric_div. The patch includes a numeric version, and I take care to short-circuit everything I can. > I'd be sort of inclined to have the swap in the other implementations > just to keep the algorithms as much alike as possible. They can't quite be the same behavior because numeric doesn't have the unrepresentable -INT_MIN problem, and integers don't have NaN. -- Vik Fearing
Re: sidewinder has one failure
On 2020-01-04 01:15, Tom Lane wrote: =?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= writes: I think Tom Lane found the "problem". It has to do with the semaphores taking up FD's. Hm, no, because: Yes, saw that after I posted my answer. Sure. I compiled pgsql 12 and this is the complete logfile after starting up the server the first time with log_min_messages=debug2: 2020-01-04 01:03:14.492 CET [14906] DEBUG: max_safe_fds = 984, usable_fds = 1000, already_open = 6 That's pretty much the same thing we see on most other platforms. Plus your configure log shows that SysV semaphores were selected, and those don't eat FDs. Yes, it looks "normal". Apparently, in the environment of that TAP test, the server has more open FDs at this point than it does when running "normally". I have no idea what the additional FDs might be. Well it's running under cron if that makes a difference and what is the TAP-test using? perl? /Mikael
Re: Greatest Common Divisor
Andres Freund writes: > On 2020-01-03 18:49:18 -0500, Tom Lane wrote: >> On a machine with single-cycle divide, it's likely that the extra >> compare-and-branch is a net loss. > Which architecture has single cycle division? I think it's way above > that, based on profiles I've seen. And Agner seems to back me up: > https://www.agner.org/optimize/instruction_tables.pdf > That lists a 32/64 idiv with a latency of ~26/~42-95 cycles, even on a > moder uarch like skylake-x. Huh. I figured Intel would have thrown sufficient transistors at that problem by now. But per that result, it's worth having the swap step even on CISC, never mind RISC. regards, tom lane
Re: sidewinder has one failure
=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= writes: > I think Tom Lane found the "problem". It has to do with the semaphores > taking up FD's. Hm, no, because: > Sure. I compiled pgsql 12 and this is the complete logfile after > starting up the server the first time with log_min_messages=debug2: > 2020-01-04 01:03:14.492 CET [14906] DEBUG: max_safe_fds = 984, > usable_fds = 1000, already_open = 6 That's pretty much the same thing we see on most other platforms. Plus your configure log shows that SysV semaphores were selected, and those don't eat FDs. Apparently, in the environment of that TAP test, the server has more open FDs at this point than it does when running "normally". I have no idea what the additional FDs might be. regards, tom lane
Re: Greatest Common Divisor
Hi, On 2020-01-03 18:49:18 -0500, Tom Lane wrote: > On some older RISC architectures, integer division is really slow, like > slower than floating-point. I'm not sure if that's true on any platform > people still care about though. In recent years, CPU architects have been > able to throw all the transistors they needed at such problems. On a > machine with single-cycle divide, it's likely that the extra > compare-and-branch is a net loss. Which architecture has single cycle division? I think it's way above that, based on profiles I've seen. And Agner seems to back me up: https://www.agner.org/optimize/instruction_tables.pdf That lists a 32/64 idiv with a latency of ~26/~42-95 cycles, even on a moder uarch like skylake-x. Greetings, Andres Freund
Re: sidewinder has one failure
On 2020-01-03 15:48, Amit Kapila wrote: On Fri, Jan 3, 2020 at 7:03 PM Amit Kapila wrote: I debugged on HEAD and found that we are closing all the files (like postgresql.conf, postgresql.auto.conf, etc.) that got opened before set_max_safe_fds. I think on HEAD the 3 already opened files are basically stdin, stdout, stderr. It is still not clear why on some other versions it shows different number of already opened files. I think Tom Lane found the "problem". It has to do with the semaphores taking up FD's. We can easily see the number of already opened files by changing the error level from DEBUG2 to LOG for elog message in set_max_safe_fds. It is not very clear to me how many files we can expect to be kept open during startup? Can the number vary on different setups? Hm, where does it get the limit from? Is it something we set? Why is this machine different from everybody else when it comes to this limit? Mikael, is it possible for you to set log_min_messages to DEBUG2 on your machine and start the server. You must see a line like: "max_safe_fds = 984, usable_fds = 1000, already_open = 6". Is it possible to share that information? This is just to confirm if the already_open number is 7 on your machine. Sure. I compiled pgsql 12 and this is the complete logfile after starting up the server the first time with log_min_messages=debug2: 2020-01-04 01:03:14.484 CET [14906] DEBUG: registering background worker "logical replication launcher" 2020-01-04 01:03:14.484 CET [14906] LOG: starting PostgreSQL 12.1 on x86_64-unknown-netbsd7.0, compiled by gcc (nb2 20150115) 4.8.4, 64-bit 2020-01-04 01:03:14.484 CET [14906] LOG: listening on IPv6 address "::1", port 5432 2020-01-04 01:03:14.484 CET [14906] LOG: listening on IPv4 address "127.0.0.1", port 5432 2020-01-04 01:03:14.485 CET [14906] LOG: listening on Unix socket "/tmp/.s.PGSQL.5432" 2020-01-04 01:03:14.491 CET [14906] DEBUG: SlruScanDirectory invoking callback on pg_notify/ 2020-01-04 01:03:14.491 CET [14906] DEBUG: removing file "pg_notify/" 2020-01-04 01:03:14.491 CET [14906] DEBUG: dynamic shared memory system will support 308 segments 2020-01-04 01:03:14.491 CET [14906] DEBUG: created dynamic shared memory control segment 2134641633 (7408 bytes) 2020-01-04 01:03:14.492 CET [14906] DEBUG: max_safe_fds = 984, usable_fds = 1000, already_open = 6 2020-01-04 01:03:14.493 CET [426] LOG: database system was shut down at 2020-01-04 01:00:15 CET 2020-01-04 01:03:14.493 CET [426] DEBUG: checkpoint record is at 0/15F15B8 2020-01-04 01:03:14.493 CET [426] DEBUG: redo record is at 0/15F15B8; shutdown true 2020-01-04 01:03:14.493 CET [426] DEBUG: next transaction ID: 486; next OID: 12974 2020-01-04 01:03:14.493 CET [426] DEBUG: next MultiXactId: 1; next MultiXactOffset: 0 2020-01-04 01:03:14.493 CET [426] DEBUG: oldest unfrozen transaction ID: 479, in database 1 2020-01-04 01:03:14.493 CET [426] DEBUG: oldest MultiXactId: 1, in database 1 2020-01-04 01:03:14.493 CET [426] DEBUG: commit timestamp Xid oldest/newest: 0/0 2020-01-04 01:03:14.493 CET [426] DEBUG: transaction ID wrap limit is 2147484126, limited by database with OID 1 2020-01-04 01:03:14.493 CET [426] DEBUG: MultiXactId wrap limit is 2147483648, limited by database with OID 1 2020-01-04 01:03:14.493 CET [426] DEBUG: starting up replication slots 2020-01-04 01:03:14.493 CET [426] DEBUG: starting up replication origin progress state 2020-01-04 01:03:14.493 CET [426] DEBUG: MultiXactId wrap limit is 2147483648, limited by database with OID 1 2020-01-04 01:03:14.493 CET [426] DEBUG: MultiXact member stop limit is now 4294914944 based on MultiXact 1 2020-01-04 01:03:14.494 CET [14906] DEBUG: starting background worker process "logical replication launcher" 2020-01-04 01:03:14.494 CET [14906] LOG: database system is ready to accept connections 2020-01-04 01:03:14.495 CET [9809] DEBUG: autovacuum launcher started 2020-01-04 01:03:14.496 CET [11463] DEBUG: received inquiry for database 0 2020-01-04 01:03:14.496 CET [11463] DEBUG: writing stats file "pg_stat_tmp/global.stat" 2020-01-04 01:03:14.497 CET [7890] DEBUG: logical replication launcher started 2020-01-04 01:03:14.498 CET [28096] DEBUG: checkpointer updated shared memory configuration values /Mikael
Re: Greatest Common Divisor
Vik Fearing writes: > On 03/01/2020 20:14, Fabien COELHO wrote: >> The point of swapping is to a void possibly expensive modulo, but this >> should be done on absolute values, otherwise it may not achieve its >> purpose as stated by the comment? > Ah, true. How widespread are these architectures that need this special > treatment? Is it really worth handling? On some older RISC architectures, integer division is really slow, like slower than floating-point. I'm not sure if that's true on any platform people still care about though. In recent years, CPU architects have been able to throw all the transistors they needed at such problems. On a machine with single-cycle divide, it's likely that the extra compare-and-branch is a net loss. Might be worth checking it on ARM in particular, as being a RISC architecture that's still popular. Also, if we end up having a "numeric" implementation, it absolutely is worth it for that, because there is nothing cheap about numeric_div. I'd be sort of inclined to have the swap in the other implementations just to keep the algorithms as much alike as possible. regards, tom lane
Re: Greatest Common Divisor
On 2020-01-03 18:00:01 -0500, Tom Lane wrote: > Alvaro Herrera writes: > > On 2020-Jan-03, Robert Haas wrote: > >> Then every time we add a function, or anything else, we can bikeshed > >> about whether it should go in pg_catalog or pg_extra! > > > Yeah, I was just thinking about that :-) I was thinking that all > > standard-mandated functions, as well as system functions, should be in > > pg_catalog; and otherwise stuff should not get in the user's way. > > I think that ship sailed a long time ago, frankly. > > Why is it that this particular proposal is such a problem that we > need to redesign how we add features? There are currently 2977 > rows in a default installation's pg_proc, with 2447 unique values > of proname. Certainly at least a couple of thousand of them are not > standard-mandated; despite which there are only 357 named 'pg_something'. > gcd and/or lcm are not going to move the needle noticeably. > > I'd also submit that just pushing a bunch of built-in stuff into a > schema that's behind the users' schema instead of in front doesn't > mean that all is magically better. There are still going to be the > same issues that make CVE-2018-1058 such a problem, but now we get > to have them in both directions not just one: > > * a system-supplied function in "pg_extra" could still capture a call > away from a user-supplied one in an earlier schema, if it is a better > match to the actual argument types; > > * malicious users now have a much better chance to capture other > people's calls to "pg_extra" functions, since they can just drop an > exact match into public. > > (BTW, I'm pretty sure we've had this conversation before. I > definitely recall a proposal to try to move functions not meant > for user consumption at all, such as index support functions, > into a whole other schema that wouldn't be in the path period. > It went nowhere, partly because those functions don't seem to > be big problems in practice.) +1 to all of this.
Re: Greatest Common Divisor
Alvaro Herrera writes: > On 2020-Jan-03, Robert Haas wrote: >> Then every time we add a function, or anything else, we can bikeshed >> about whether it should go in pg_catalog or pg_extra! > Yeah, I was just thinking about that :-) I was thinking that all > standard-mandated functions, as well as system functions, should be in > pg_catalog; and otherwise stuff should not get in the user's way. I think that ship sailed a long time ago, frankly. Why is it that this particular proposal is such a problem that we need to redesign how we add features? There are currently 2977 rows in a default installation's pg_proc, with 2447 unique values of proname. Certainly at least a couple of thousand of them are not standard-mandated; despite which there are only 357 named 'pg_something'. gcd and/or lcm are not going to move the needle noticeably. I'd also submit that just pushing a bunch of built-in stuff into a schema that's behind the users' schema instead of in front doesn't mean that all is magically better. There are still going to be the same issues that make CVE-2018-1058 such a problem, but now we get to have them in both directions not just one: * a system-supplied function in "pg_extra" could still capture a call away from a user-supplied one in an earlier schema, if it is a better match to the actual argument types; * malicious users now have a much better chance to capture other people's calls to "pg_extra" functions, since they can just drop an exact match into public. (BTW, I'm pretty sure we've had this conversation before. I definitely recall a proposal to try to move functions not meant for user consumption at all, such as index support functions, into a whole other schema that wouldn't be in the path period. It went nowhere, partly because those functions don't seem to be big problems in practice.) regards, tom lane
Re: Greatest Common Divisor
On 03/01/2020 20:14, Fabien COELHO wrote:
>
> Bonsoir Vik,
>
> +int4gcd_internal(int32 arg1, int32 arg2)
> +{
> + int32 swap;
> +
> + /*
> + * Put the greater value in arg1.
> + * This would happen automatically in the loop below, but
> avoids an
> + * expensive modulo simulation on some architectures.
> + */
> + if (arg1 < arg2)
> + {
> + swap = arg1;
> + arg1 = arg2;
> + arg2 = swap;
> + }
>
>
> The point of swapping is to a void possibly expensive modulo, but this
> should be done on absolute values, otherwise it may not achieve its
> purpose as stated by the comment?
Ah, true. How widespread are these architectures that need this special
treatment? Is it really worth handling?
> I'm unsure about gcd(INT_MIN, 0) should error. Possibly 0 would be nicer?
What justification for that do you have?
--
Vik Fearing
Re: Greatest Common Divisor
On 2020-Jan-03, Robert Haas wrote: > On Fri, Jan 3, 2020 at 4:11 PM Alvaro Herrera > wrote: > > Maybe a very simple solution is indeed to have a separate pg_math or > > pg_extra or whatever, which by default is *last* in the search_path. > > That would make a user's gcd() be chosen preferently, if one exists. > > Then every time we add a function, or anything else, we can bikeshed > about whether it should go in pg_catalog or pg_extra! Yeah, I was just thinking about that :-) I was thinking that all standard-mandated functions, as well as system functions, should be in pg_catalog; and otherwise stuff should not get in the user's way. > FWIW, EnterpriseDB has something like this for Advanced Server, and it > actually adds a fair amount of complexity, much of it around > OverrideSearchPath. It's not unmanageable, but it's not trivial, > either. Oh, hmm. okay. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Greatest Common Divisor
On Fri, Jan 3, 2020 at 4:11 PM Alvaro Herrera wrote: > Maybe a very simple solution is indeed to have a separate pg_math or > pg_extra or whatever, which by default is *last* in the search_path. > That would make a user's gcd() be chosen preferently, if one exists. Then every time we add a function, or anything else, we can bikeshed about whether it should go in pg_catalog or pg_extra! FWIW, EnterpriseDB has something like this for Advanced Server, and it actually adds a fair amount of complexity, much of it around OverrideSearchPath. It's not unmanageable, but it's not trivial, either. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Greatest Common Divisor
On Fri, Jan 3, 2020 at 3:51 PM Merlin Moncure wrote: > Is that right? Default search_path is for pg_catalog to resolve before > public. Lightly testing with a hand rolled pg_advisory_lock > implementation that raise a notice, my default database seemed to > prefer the build in function. Maybe I'm not following you. Nope, I'm just wrong. Sorry. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Online checksums patch - once again
On Mon, Dec 16, 2019 at 10:16 AM Daniel Gustafsson wrote:
> If reviewers think this version is nearing completion, then a v16 should
> address the comment below, but as this version switches its underlying
> infrastructure it seemed usefel for testing still.
I think this patch still needs a lot of work.
- doPageWrites = (Insert->fullPageWrites || Insert->forcePageWrites);
+ doPageWrites = (Insert->fullPageWrites || Insert->forcePageWrites ||
DataChecksumsInProgress());
This will have a small performance cost in a pretty hot code path. Not
sure that it's enough to worry about, though.
-DataChecksumsEnabled(void)
+DataChecksumsNeedWrite(void)
{
Assert(ControlFile != NULL);
return (ControlFile->data_checksum_version > 0);
}
This seems troubling, because data_checksum_version can now change,
but you're still accessing it without a lock. This complain applies
likewise to a bunch of related functions in xlog.c as well.
+ elog(ERROR, "Checksums not in inprogress mode");
Questionable capitalization and punctuation.
+void
+SetDataChecksumsOff(void)
+{
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
+
+ ControlFile->data_checksum_version = 0;
+ UpdateControlFile();
+ LWLockRelease(ControlFileLock);
+ WaitForProcSignalBarrier(EmitProcSignalBarrier(PROCSIGNAL_BARRIER_CHECKSUM));
+
+ XlogChecksums(0);
+}
This looks racey. Suppose that checksums are on. Other backends will
see that checksums are disabled as soon as
ControlFile->data_checksum_version = 0 happens, and they will feel
free to write blocks without checksums. Now we crash, and those blocks
exist on disk even though the on-disk state still otherwise shows
checksums fully enabled. It's a little better if we stop reading
data_checksum_version without a lock, because now nobody else can see
the updated state until we've actually updated the control file. But
even then, isn't it strange that writes of non-checksummed stuff could
appear or be written to disk before XlogChecksums(0) happens? If
that's safe, it seems like it deserves some kind of comment.
+ /*
+ * If we reach this point with checksums in inprogress state, we notify
+ * the user that they need to manually restart the process to enable
+ * checksums. This is because we cannot launch a dynamic background worker
+ * directly from here, it has to be launched from a regular backend.
+ */
+ if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_INPROGRESS_VERSION)
+ ereport(WARNING,
+ (errmsg("checksum state is \"inprogress\" with no worker"),
+ errhint("Either disable or enable checksums by calling the
pg_disable_data_checksums() or pg_enable_data_checksums()
functions.")));
This seems pretty half-baked.
+ (errmsg("could not start checksumhelper: has been canceled")));
+ (errmsg("could not start checksumhelper: already running")));
+ (errmsg("failed to start checksum helper launcher")));
These seem rough around the edges. Using an internal term like
'checksumhelper' in a user-facing error message doesn't seem great.
Generally primary error messages are phrased as a single utterance
where we can, rather than colon-separated fragments like this. The
third message calls it 'checksum helper launcher' whereas the other
two call it 'checksumhelper'. It also isn't very helpful; I don't
think most people like a message saying that something failed with no
explanation given.
+ elog(DEBUG1, "Checksumhelper skipping relation %d as it no longer
exists", relationId);
Here's another way to spell 'checksumhelper', and this time it refers
to the worker rather than the launcher. Also, relation IDs are OIDs,
so need to be printed with %u, and usually we try to print names if
possible. Also, this message, like a lot of messages in this patch,
begins with a capital letter and does not end with a period. That is
neither the style for primary messages nor the style for detail
messages. As these are primary messages, the primary message style
should be used. That style is no capital and no period.
+ if (!RegisterDynamicBackgroundWorker(, _handle))
+ {
+ ereport(LOG,
+ (errmsg("failed to start worker for checksumhelper in \"%s\"",
+ db->dbname)));
+ return FAILED;
+ }
I don't think having constants with names like
SUCCESSFUL/ABORTED/FAILED is a very good idea. Too much chance of name
collisions. I suggest adding a prefix.
Also, the retry logic here doesn't look particularly robust.
RegisterDynamicBackgroundWorker will fail if all slots are available;
if that happens twice for the same database, once on first attempting
it and again when retrying it, the whole process fails, all state is
lost, and all work has to be redone. That seems neither particularly
unlikely nor pleasant.
+ if (DatabaseList == NIL || list_length(DatabaseList) == 0)
I don't think that the second half of this test serves any purpose.
+ snprintf(activity, sizeof(activity), "Waiting for current
transactions to finish (waiting for %d)", waitforxid);
%u here too.
+ if (pgc->relpersistence == 't')
Use the relevant constant.
+
Re: Removal of support for OpenSSL 0.9.8 and 1.0.0
> On 3 Jan 2020, at 07:49, Michael Paquier wrote: > > On Thu, Jan 02, 2020 at 11:45:37PM -0500, Tom Lane wrote: >> Ah. The CF app doesn't understand that (and hence the cfbot ditto), >> so you might want to repost just the currently-proposed patch to get >> the cfbot to try it. > > Yes, let's do that. Here you go with a v2. While on it, I have > noticed in the docs a mention to OpenSSL 1.0.0 regarding our > sslcompression parameter in libpq, so a paragraph can be removed. LGTM, switching to ready for committer. cheers ./daniel
Re: Greatest Common Divisor
On 1/3/20 4:10 PM, Alvaro Herrera wrote: > Maybe a very simple solution is indeed to have a separate pg_math or > pg_extra or whatever, which by default is *last* in the search_path. > That would make a user's gcd() be chosen preferently, if one exists. I'm liking the direction this is going. Regards, -Chap
Re: Greatest Common Divisor
On 2020-Jan-03, Merlin Moncure wrote: > On Fri, Jan 3, 2020 at 1:32 PM Robert Haas wrote: > > True, but because of the way search_path is typically set, they'd > > probably continue to get their own version anyway, so I'm not sure > > what the problem is. > > Is that right? Default search_path is for pg_catalog to resolve before > public. Lightly testing with a hand rolled pg_advisory_lock > implementation that raise a notice, my default database seemed to > prefer the build in function. Maybe I'm not following you. Maybe a very simple solution is indeed to have a separate pg_math or pg_extra or whatever, which by default is *last* in the search_path. That would make a user's gcd() be chosen preferently, if one exists. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Greatest Common Divisor
On 1/3/20 3:09 PM, Peter Eisentraut wrote: > Geometry is generally in scope, though, for Postgres specifically and > for databases in general. > > Abstract algebra is not in scope, so far, and we still haven't been told > the use case for this. It's funny, I think I've used gcd and lcm in real life way more often than sinh and cosh, maybe even as often as sin and cos. For example, how many times around will I have to go with this engine crankshaft to be able to confirm the painted links on the timing chain really do line up with the sprocket marks? (Need to count the sprocket teeth and the chain links.) Or, if I'm cycling through two different-length tuple stores, how many times before the same tuples coincide again? That isn't a question I've yet had an occasion to face, but I don't have to squint real hard to imagine it arising in a database in some situation or other. This is just me riffing, as of course I'm not the person who had such a pressing use case as to justify sitting down to write the patch. Another funny thing: this message sent me googling just to indulge my own "is gcd more abstract algebra or number theory?" quibble*, and I ended up discovering there are more algorithms for it than the Euclidean one I remember. There's a binary one using only ands, subtractions, and shifts, asymptotically the same as Euclid but perhaps somewhat faster: https://en.wikipedia.org/wiki/Binary_GCD_algorithm It looks fairly simple to code up, if not quite as short as Euclid. There's at least one specific to representations like numeric: https://en.wikipedia.org/wiki/Lehmer%27s_GCD_algorithm ... considerably more effort to implement though. It might be possible, if there are crypto libraries we're already linking to for other reasons like SSL, there could be good big-number gcd implementations already in there. Regards, -Chap * maybe I've decided to call it number theory if the things being gcd'd are integers, abstract algebra if they belong to other commutative rings.
Re: Greatest Common Divisor
On Fri, Jan 3, 2020 at 1:32 PM Robert Haas wrote: > > On Fri, Jan 3, 2020 at 2:27 PM Chapman Flack wrote: > > On 1/3/20 2:11 PM, Robert Haas wrote: > > > and moving things to another schema does not help with that. It does > > > potentially help with the namespace pollution issue, but how much of > > > an issue is that anyway? Unless you've set up an unusual search_path > > > configuration, your own schemas probably precede pg_catalog in your > > > search path, besides which it seems unlikely that many people have a > > > gcd() function that does anything other than take the greatest common > > > divisor. > > > > As seen in this thread though, there can be edge cases of "take the > > greatest common divisor" that might not be identically treated in a > > thoroughly-reviewed addition to core as in someone's hastily-rolled > > local version. > > True, but because of the way search_path is typically set, they'd > probably continue to get their own version anyway, so I'm not sure > what the problem is. Is that right? Default search_path is for pg_catalog to resolve before public. Lightly testing with a hand rolled pg_advisory_lock implementation that raise a notice, my default database seemed to prefer the build in function. Maybe I'm not following you. > There are counter-arguments to that, though. Maintaining a lot of > extensions with only one or two functions in them is a nuisance. > Having things installed by default is convenient for wanting to use > them. Maintaining contrib code so that it works whether or not the SQL > definitions have been updated via ALTER EXTENSION .. UPDATE takes some > work and thought, and sometimes we screw it up. If the external contract changes (which seems likely for gcd) than I would much rather have the core team worry about this than force your users to worry about it, which is what putting the function in core would require them to do (if version < x call it this way, > y then that way etc). This is exactly why we shouldn't be putting non standard items in core (maybe excepting some pg_ prefixed administration functions). Now, it's quite unfair to $OP to saddle his proposal and patch with the broader considerations of core/extension packaging, so if some kind of rational framework can be applied to the NEXT submission, or a least a discussion about this can start, those are all good options. But we need to start from somewhere, and moving forward with, "If it's not sql standard or prefixed with pg_, it ought not to be in pg_catalog" might be a good way to open the discussion. merlin
Re: Assigning ROW variable having NULL value to RECORD type variable doesn't give any structure to the RECORD variable.
Robert Haas writes: > On Wed, Jan 1, 2020 at 10:50 AM Ashutosh Sharma wrote: >> I know this is expected to happen considering the changes done in >> above commit because from this commit onwards, NULL value assigned to >> any row variable represents a true NULL composite value before this >> commit it used to be a tuple with each column having null value in it. >> But, the point is, even if the row variable is having a NULL value it >> still has a structure associated with it. Shouldn't that structure be >> transferred to RECORD variable when it is assigned with a ROW type >> variable ? Can we consider this behaviour change as a side effect of >> the improvement done in the RECORD type of variable? > I'm not an expert on this topic. However, I *think* that you're trying > to distinguish between two things that are actually the same. If it's > a "true NULL," it has no structure; it's just NULL. If it has a > structure, then it's really a composite value with a NULL in each > defined column, i.e. (NULL, NULL, NULL, ...) for some row type rather > than just NULL. Yeah. In general, we can't do this, because a null value of type RECORD simply hasn't got any information about what specific rowtype might be involved. In the case where the null is of a named composite type, rather than RECORD, we could choose to act differently ... but I'm not really sure that such a change would be an improvement and not just a decrease in consistency. In any case, plpgsql's prior behavior was an implementation artifact with very little to recommend it. As a concrete example, consider create table t1(a int, b text); do $$ declare x t1; r record; begin x := null; r := x; raise notice 'r.a = %', r.a; end $$; do $$ declare r record; begin r := null::t1; raise notice 'r.a = %', r.a; end $$; I assert that in any sanely-defined semantics, these two examples should give the same result. In v11 and up, they both give 'record "r" is not assigned yet' ... but in prior versions, they gave different results. I do not want to go back to that. On the other hand, we now have do $$ declare x t1; r record; begin x := null; r := x; raise notice 'x.a = %', x.a; raise notice 'r.a = %', r.a; end $$; which gives NOTICE: x.a = ERROR: record "r" is not assigned yet which is certainly also inconsistent. The variable declared as being type t1 behaves, for this purpose, as if it contained "row(null,null)" not just a simple null. But if you print it, or assign it to something else as a whole, you'll find it just contains a simple null. One way to see that these are different states is to do do $$ declare x t1; begin x := null; raise notice 'x = %', x; end$$; NOTICE: x = versus do $$ declare x t1; begin x := row(null,null); raise notice 'x = %', x; end$$; NOTICE: x = (,) And, if you assign a row of nulls to a record-type variable, that works: do $$ declare x t1; r record; begin x := row(null,null); r := x; raise notice 'x.a = %', x.a; raise notice 'r.a = %', r.a; end $$; which gives NOTICE: x.a = NOTICE: r.a = If we were to change this behavior, I think it would be tantamount to sometimes expanding a simple null to a row of nulls, and I'm not sure that's a great idea. The SQL standard is confusing in this respect, because it seems that at least the "x IS [NOT] NULL" construct is defined to consider both a "simple NULL" and ROW(NULL,NULL,...) as "null". But we've concluded that other parts of the spec do allow for a distinction (I'm too lazy to search the archives for relevant discussions, but there have been some). The two things are definitely different implementation-wise, so it would be hard to hide the difference completely. Another fun fact is that right now, assignment of any null value to a composite plpgsql variable works the same: you can assign a simple null of some other composite type, or even a scalar null, and behold you get a null composite value without any error. That's because exec_assign_value's DTYPE_REC case pays no attention to the declared type of the source value once it's found to be null. Thus do $$ declare x t1; begin x := 42; raise notice 'x = %', x; end$$; ERROR: cannot assign non-composite value to a record variable do $$ declare x t1; begin x := null::int; raise notice 'x = %', x; end$$; NOTICE: x = That's pretty bizarre, and I don't think I'd agree with adopting those semantics if we were in a green field. But if we start paying attention to the specific type of a null source value, I bet we're going to break some code that works today. Anyway, maybe this area could be improved, but I'm not fully convinced. I definitely do not subscribe to the theory that we need to make it work like v10 again. regards, tom lane
Re: global / super barriers (for checksums)
On Mon, Dec 9, 2019 at 10:42 AM Robert Haas wrote: > > Hm. In my mental model it would be useful for barrier "processors" to > > not acknowledge the state change at certain points. Imagine e.g. needing > > to efficiently wait till all backends have processed a config file > > reload - since we don't reload while a query is being processed, we > > should be able to not ack the barrier at that point. > > Yeah, you mentioned this before, but I think we could leave it for > later. I think it's going to be complex. I suppose the way to do it > would be to add an argument to ProcessProcSignalBarrier() that lets > you specify which barrier events you're OK with processing from the > current point in the code. However, that will mean that whenever > somebody adds a new barrier type, they have got to go through all of > those callers and think about whether they should add their new > barrier type into the flags or not. If we try to do the specific thing > you're talking about with config-file processing, it will also mean > that we could be waiting MUCH longer for acknowledgements, which I > think might have pretty far-reaching ramifications. You might get > stuck waiting for that barrier to be absorbed for hours, and that > would also impact later barriers, since you won't see the generation > bump to 42 until it goes through 40 and 41. That sounds fairly awful. > You could try to work around it by looking at which barrier flags are > set, but I think that has ABA problems. I might have rejected this idea too hastily. At any rate, I have a new view on it having studied the issue a bit more. In the case of ALTER SYSTEM READ ONLY, the big problem is that we can't afford to find out that we're unable to emit WAL after we've already entered a critical section, because "ERROR: system is read only" or similar will get promoted to PANIC due to the critical section and that will be sad. (Before someone asks, not promoting the ERROR to PANIC would be unsafe, even for this specific case.) Some WAL records don't use a critical section, though it's recently been proposed[1] that we add a critical section in at least some of the cases that currently lack one. For those, I think we can just upgrade the existing test-and-elog cases in XLogInsert() and XLogBeginInsert() to ereport() and call it good. But the cases that do have a critical section are harder. Currently, I see only the following two options: (1) Check just before entering a critical section whether or not the system is read only, and if so, error out. Once we've entered a critical section, ProcessInterrupts() will not do anything, so at that point we're safe. This could be done either by making START_CRIT_SECTION() itself do it or by finding every critical section that is used for inserting WAL and adding a call just beforehand. The latter is probably better, as some critical sections appear to be used for other purposes; see PGSTAT_BEGIN_WRITE_ACTIVITY in particular. But it means changing things in a bunch of places, adding an if-test. That wouldn't add *much* overhead, but it's not quite zero. (2) Accept barrier events for read-only/read-write changes only at command boundaries. In that case, we only need to check for a read only state around the places where we currently do PreventCommandIfReadOnly and similar, and the additional overhead should be basically nil. However, this seems pretty unappealing to me, because it means that if you try to make the system READ ONLY, it might run for hours before actually going read only. If you're trying to make the system read only because the master has become isolated from the rest of your network, that's not a fast enough response. In general, I think the problem here is to avoid TOCTTOU problems. I think the checksums patch has this problem too. For instance, the changes to basebackup.c in that function check DataChecksumsNeedVerify() only once per file, but what is to say that a call to ProcessInterrupts() couldn't happen in the middle of sending a file, changing prevailing value? If someone sets checksums off, and everyone acknowledges that they're off, then backends may begin writing blocks without setting checksums, and then this code will potentially try to verify checksums in a block written without them. That patch also modifies the definition of XLogHintBitIsNeeded(), so e.g. log_heap_visible is wrong if a CHECK_FOR_INTERRUPTS() can happen in XLogRegisterBuffer(). I don't think it can currently, but it seems like a heck of a fragile assumption, and I don't see how we can be sure that there's no test for XLogHintBitIsNeeded() that does CHECK_FOR_INTERRUPTS() between where it's tested and where it does something critical with the result of that test. At the moment, the ALTER SYSTEM READ ONLY problem appears to me to be the lesser of the two (although I may be wrong). Assuming people are OK with inserting an additional check before each xlog-writing critical section, we can just go do that and then I think
Re: color by default
On Tue, Dec 31, 2019 at 8:35 AM Tom Lane wrote: > Peter Eisentraut writes: > > With the attached patch, I propose to enable the colored output by > > default in PG13. > > FWIW, I shall be setting NO_COLOR permanently if this gets committed. > I wonder how many people there are who actually *like* colored output? > I find it to be invariably less readable than plain B text. > > I find color massively useful for grep and its variants, where the hit can show up anywhere on the line. It was also kind of useful for git, especially "git grep", but on my current system git's colorizing seems hopelessly borked, so I had to turn it off. But I turned PG_COLOR on and played with many commands, and must say I don't really see much of a point. When most of these command fail, they only generate a few lines of output, and it isn't hard to spot the error message. When pg_restore goes wrong, you get a lot of messages but colorizing them isn't really helpful. I don't need 'error' to show up in red in order to know that I have a lot of errors, especially since the lines which do report errors always have 'error' as the 2nd word on the line, where it isn't hard to spot. If it could distinguish the important errors from unimportant errors, that would be more helpful. But if it could reliably do that, why print the unimportant ones at all? It doesn't seem like this is useful enough to have it on by default, and without it being on by default there is no point in having NO_COLOR to turn if off. There is something to be said for going with the flow, but the "emerging standard" seems like it has quite a bit further to emerge before I think that would be an important reason. Cheers, Jeff
Re: Greatest Common Divisor
On 2020-01-03 16:22, Tom Lane wrote: Peter Eisentraut writes: On 2020-01-02 15:50, Dean Rasheed wrote: Out of curiosity, what was the original use-case for this? Yeah, I'm wondering, is this useful for any typical analytics or business application? Otherwise, abstract algebra functionality seems a bit out of scope. Nobody complained when we added sinh, cosh, tanh, asinh, acosh, atanh last year, so I'm feeling skeptical of claims that gcd should be out of scope. Geometry is generally in scope, though, for Postgres specifically and for databases in general. Abstract algebra is not in scope, so far, and we still haven't been told the use case for this. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: weird libpq GSSAPI comment
Alvaro Herrera writes: > How about this? > > * If GSSAPI is enabled and we can reach a credential cache, > * set up a handle for it; if it's operating, just send a > * GSS startup message, instead of the SSL negotiation and > * regular startup message below. Due to the way postgres handled this historically, there are two ways GSSAPI can be used: for connection encryption, and for authentication only. We perform the same dance of sending a "request packet" for GSSAPI encryption as we do for TLS encryption. So I'd like us to be precise about which one we're talking about here (encryption). The GSSAPI idiom I should have used is "can acquire credentials" (i.e., instead of "can reach a credential cache" in your proposal). There's no such thing as a "GSS startup message". After negotiating GSSAPI/TLS encryption (or failing to do so), we send the same things in all cases, which includes negotiation of authentication mechanism if any. (Negotiating GSSAPI for authentication after negotiating GSSAPI for encryption will short-circuit rather than establishing a second context, if I remember right.) I wonder if part of the confusion might be due to the synonyms we're using here for "in use". Things seem to be "got running", "set up", "operating", "negotiated", ... - maybe that's part of the barrier to understanding? Thanks, --Robbie signature.asc Description: PGP signature
Re: Assigning ROW variable having NULL value to RECORD type variable doesn't give any structure to the RECORD variable.
pá 3. 1. 2020 v 19:57 odesílatel Robert Haas napsal: > On Wed, Jan 1, 2020 at 10:50 AM Ashutosh Sharma > wrote: > > I know this is expected to happen considering the changes done in > > above commit because from this commit onwards, NULL value assigned to > > any row variable represents a true NULL composite value before this > > commit it used to be a tuple with each column having null value in it. > > But, the point is, even if the row variable is having a NULL value it > > still has a structure associated with it. Shouldn't that structure be > > transferred to RECORD variable when it is assigned with a ROW type > > variable ? Can we consider this behaviour change as a side effect of > > the improvement done in the RECORD type of variable? > > I'm not an expert on this topic. However, I *think* that you're trying > to distinguish between two things that are actually the same. If it's > a "true NULL," it has no structure; it's just NULL. If it has a > structure, then it's really a composite value with a NULL in each > defined column, i.e. (NULL, NULL, NULL, ...) for some row type rather > than just NULL. > > I have to admit that I've always found PL/pgsql to be a bit pedantic > about this whole thing. For instance: > > rhaas=# do $$declare x record; begin raise notice '%', x.a; end;$$ > language plpgsql; > ERROR: record "x" is not assigned yet > DETAIL: The tuple structure of a not-yet-assigned record is indeterminate. > CONTEXT: SQL statement "SELECT x.a" > PL/pgSQL function inline_code_block line 1 at RAISE > > But maybe it should just make x.a evaluate to NULL. It's one thing if > I have a record with columns 'a' and 'b' and I ask for column 'c'; I > guess you could call that NULL, but it feels reasonably likely to be a > programming error. But if we have no idea what the record columns are > at all, perhaps we could just assume that whatever column the user is > requesting is intended to be one of them, and that since the whole > thing is null, that column in particular is null. > I don't like this idea. We should not to invent record's fields created by reading or writing some field. At end it block any static code analyze and it can hide a errors. If we enhance a interface for json or jsonb, then this dynamic work can be done with these types. We should to distinguish between typend and untyped NULL - it has sense for me (what was proposed by Ashutosh Sharma), but I don't see any sense to go far. Regards Pavel > On the other hand, maybe that would be too lenient and lead to subtle > and hard-to-find bugs in plpgsql programs. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > >
Re: backup manifests
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Fri, Jan 3, 2020 at 12:01 PM Stephen Frost wrote: > > You're certainly intending to do *something* with the manifest, and > > while I appreciate that you feel you've come up with a complete use-case > > that this simple manifest will be sufficient for, I frankly doubt > > that'll actually be the case. Not long ago it wasn't completely clear > > that a manifest at *all* was even going to be necessary for the specific > > use-case you had in mind (I'll admit I wasn't 100% sure myself at the > > time either), but now that we're down the road of having one, I can't > > agree with the blanket assumption that we're never going to want to > > extend it, or even that it won't be necessary to add to it before this > > particular use-case is fully addressed. > > > > And the same goes for the other things that were discussed up-thread > > regarding memory context and error handling and such. > > Well, I don't know how to make you happy here. I suppose I should admit that, first off, I don't feel you're required to make me happy, and I don't think it's necessary to make me happy to get this feature into PG. Since you expressed that interest though, I'll go out on a limb and say that what would make me *really* happy would be to think about where the project should be taking pg_basebackup, what we should be working on *today* to address the concerns we hear about from our users, and to consider the best way to implement solutions to what they're actively asking for a core backup solution to be providing. I get that maybe that isn't how the world works and that sometimes we have people who write our paychecks wanting us to work on something else, and yes, I'm sure there are some users who are asking for this specific thing but I certainly don't think it's a common ask of pg_basebackup or what users feel is missing from the backup options we offer in core; we had users on this list specifically saying they *wouldn't* use this feature (referring to the differential backup stuff, of course), in fact, because of the things which are missing, which is pretty darn rare. That's what would make *me* happy. Even some comments about how to *get* there while also working towards these features would be likely to make me happy. Instead, I feel like we're being told that we need this feature badly in v13 and we're going to cut bait and do whatever is necessary to get us there. > It looks to me like > insisting on a JSON-format manifest will likely mean that this doesn't > get into PG13 or PG14 or probably PG15, because a port of all that > machinery to work in frontend code will be neither simple nor quick. I certainly understand that these things take time, sometimes quite a bit of it as the past 2 years have shown in this other little side project, and that was hacking without having to go through the much larger effort involved in getting things into PG core. That doesn't mean that kind of effort isn't worthwhile or that, because something is a bunch of work, we shouldn't spend the time on it. I do feel what you're after here is a multi-year project, and I've said before that I don't agree that this is a feature (the differential backup with pg_basebackup thing) that makes any sense going into PG at this time, but I'm also not trying to block this feature, just to share the experience that we've gotten from working in this area for quite a while and hopefully help guide the effort in PG away from pitfalls and in a good direction long-term. > If you want this to happen for this release, you've got to be willing > to settle for something that can be implemented in the time we have. I'm not sure what you're expecting here, but for my part, at least, I'm not going to be terribly upset if this feature doesn't make this release because there's an agreement and understanding that the current direction isn't a good long-term solution. Nor am I going to be terribly upset about the time that's been spent on this particular approach given that there's been no shortage of people commenting that they'd rather see an extensible format, like JSON, and has been for quite some time. All that said- one thing we've done is to consider that *we* are the ones who are writing the JSON, while also being the ones to read it- we don't need the parsing side to understand and deal with *any* JSON that might exist out there, just whatever it is the server creates/created. It may be possible to use that to simplify the parser, or perhaps at least to accept that if it ends up being given something else that it might not perform as well with it. I'm not sure how helpful that will be to you, but I recall David finding it a helpful thought. > I'm not sure whether what you and David are arguing boils down to > thinking that I'm wrong when I say that doing that is hard, or whether > you know it's hard but you just don't care because you'd rather see > the feature go nowhere than use
Re: aggregate crash
Hi, On 2019-12-27 20:13:26 +0300, Teodor Sigaev wrote: > Found crash on production instance, assert-enabled build crashes in pfree() > call, with default config. v11, v12 and head are affected, but, seems, you > need to be a bit lucky. > > The bug is comparing old and new aggregate pass-by-ref values only by > pointer value itself, despite on null flag. Any function which returns null > doesn't worry about actual returned Datum value, so that comparison isn't > enough. Test case shows bug with ExecInterpExpr() but there several similar > places (thanks Nikita Glukhov for help). > Attached patch adds check of null flag. Hm. I don't understand the problem here. Why do we need to reparent in that case? What freed the relevant value? Nor do I really understand why v10 wouldn't be affected if this actually is a problem. The relevant code is also only guarded by DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue)) > > Backtrace from v12 (note, newValue and oldValue are differ on current call, > but oldValue points into pfreed memory) : > #0 0x00c8405a in GetMemoryChunkContext (pointer=0x80a808250) at > ../../../../src/include/utils/memutils.h:130 > 130 AssertArg(MemoryContextIsValid(context)); > (gdb) bt > #0 0x00c8405a in GetMemoryChunkContext (pointer=0x80a808250) at > ../../../../src/include/utils/memutils.h:130 > #1 0x00c85ae5 in pfree (pointer=0x80a808250) at mcxt.c:1058 > #2 0x0080475e in ExecAggTransReparent (aggstate=0x80a806370, > pertrans=0x80a87e830, newValue=34535940744, newValueIsNull=false, > oldValue=34535932496, oldValueIsNull=false) > at execExprInterp.c:4209 > #3 0x007ff51f in ExecInterpExpr (state=0x80a87f4d8, > econtext=0x80a8065a8, isnull=0x7fffd7b7) at execExprInterp.c:1747 > #4 0x0082c12b in ExecEvalExprSwitchContext (state=0x80a87f4d8, > econtext=0x80a8065a8, isNull=0x7fffd7b7) at > ../../../src/include/executor/executor.h:308 > #5 0x0082bc0f in advance_aggregates (aggstate=0x80a806370) at > nodeAgg.c:679 > #6 0x0082b8a6 in agg_retrieve_direct (aggstate=0x80a806370) at > nodeAgg.c:1847 > #7 0x00828782 in ExecAgg (pstate=0x80a806370) at nodeAgg.c:1572 > #8 0x0080e712 in ExecProcNode (node=0x80a806370) at > ../../../src/include/executor/executor.h:240 > How to reproduce: > http://sigaev.ru/misc/xdump.sql.bz2 > bzcat xdump.sql.bz2 | psql postgres && psql postgres < x.sql It should be possible to create a smaller reproducer... It'd be good if a bug fix for this were committed with a regression test. > diff --git a/src/backend/executor/execExprInterp.c > b/src/backend/executor/execExprInterp.c > index 034970648f3..3b5333716d4 100644 > --- a/src/backend/executor/execExprInterp.c > +++ b/src/backend/executor/execExprInterp.c > @@ -1743,7 +1743,8 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, > bool *isnull) >* expanded object that is already a child of the > aggcontext, >* assume we can adopt that value without copying it. >*/ > - if (DatumGetPointer(newVal) != > DatumGetPointer(pergroup->transValue)) > + if (DatumGetPointer(newVal) != > DatumGetPointer(pergroup->transValue) || > + fcinfo->isnull != pergroup->transValueIsNull) > newVal = ExecAggTransReparent(aggstate, > pertrans, > > newVal, fcinfo->isnull, > > pergroup->transValue, I'd really like to avoid adding additional branches to these paths. Greetings, Andres Freund
Re: Greatest Common Divisor
On Fri, Jan 3, 2020 at 2:27 PM Chapman Flack wrote: > On 1/3/20 2:11 PM, Robert Haas wrote: > > and moving things to another schema does not help with that. It does > > potentially help with the namespace pollution issue, but how much of > > an issue is that anyway? Unless you've set up an unusual search_path > > configuration, your own schemas probably precede pg_catalog in your > > search path, besides which it seems unlikely that many people have a > > gcd() function that does anything other than take the greatest common > > divisor. > > As seen in this thread though, there can be edge cases of "take the > greatest common divisor" that might not be identically treated in a > thoroughly-reviewed addition to core as in someone's hastily-rolled > local version. True, but because of the way search_path is typically set, they'd probably continue to get their own version anyway, so I'm not sure what the problem is. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Greatest Common Divisor
On 1/3/20 2:11 PM, Robert Haas wrote: > and moving things to another schema does not help with that. It does > potentially help with the namespace pollution issue, but how much of > an issue is that anyway? Unless you've set up an unusual search_path > configuration, your own schemas probably precede pg_catalog in your > search path, besides which it seems unlikely that many people have a > gcd() function that does anything other than take the greatest common > divisor. As seen in this thread though, there can be edge cases of "take the greatest common divisor" that might not be identically treated in a thoroughly-reviewed addition to core as in someone's hastily-rolled local version. Regards, -Chap
Re: weird libpq GSSAPI comment
Stephen Frost writes: > Greetings, > > (I've added Robbie to this thread, so he can correct me if/when I go > wrong in my descriptions regarding the depths of GSSAPI ;) Hi, appreciate the CC since I'm not subscribed anymore. Thanks for your patience while I was PTO. > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: >> I found this comment in fe-connect.c: >> >> /* >> * If GSSAPI is enabled and we have a credential cache, try >> to >> * set it up before sending startup messages. If it's >> already >> * operating, don't try SSL and instead just build the >> startup >> * packet. >> */ >> >> I'm not sure I understand this correctly. Why does it say "just >> build the startup" packet about the SSL thing, when in reality the >> SSL block below is unrelated to the GSS logic? If I consider that >> SSL is just a typo for GSS, then the comment doesn't seem to describe >> the logic either, because what it does is go to >> CONNECTION_GSS_STARTUP state which *doesn't* "build the startup >> packet" in the sense of pqBuildStartupPacket2/3, but instead it just >> does pqPacketSend (which is what the SSL block below calls "request >> SSL instead of sending the startup packet"). > > SSL there isn't a typo for GSS. The "startup packet" being referred to > in that comment is specifically the "request GSS" that's sent via the > following pqPacketSend, not the pqBuildStartupPacket one. I agree > that's a bit confusing and we should probably reword that, since > "Startup Packet" has a specific meaning in this area of the code. The comment refers to the first `if`, mostly. The idea is that we want to check whether we *can* perform GSSAPI negotiation, and skip it otherwise - which is determined by attempting to acquire credentials. There will be false positives for this check, but no false negatives, and it's a step that GSSAPI performs as part of negotiation anyway so it costs us basically nothing since we cache the result. The "startup packet" the comment refers to is that just below on 2867 - the pqBuildStartupPacket one. The flow is: 1. Set up GSSAPI, if possible. 2. Set up TLS, if possible. 3. Send startup packet. >> Also, it says "... and we have a credential cache, try to set it >> up..." but I think it should say "if we *don't* have a credential >> cache". > > No, we call pg_GSS_have_cred_cache() here, which goes on to call > gss_acquire_cred(), which, as the comment above that function says, > checks to see if we can acquire credentials by making sure that we *do* > have a credential cache. If we *don't* have a credential cache, then we > fall back to SSL (and then to non-SSL). Right. >> Now that I've read this code half a dozen times, I think I'm starting >> to vaguely understand how it works, but I would have expected the >> comment to explain it so that I didn't have to do that. > > I'm concerned that you don't quite understand it though, I'm afraid. Same. I tried to model after the TLS code for this. That has the following comment: If SSL is enabled and we haven't already got it running, request it instead of sending the startup message. >> Can we discuss a better wording for this comment? I wrote this, but I >> don't think it captures all the nuances in this code: >> >> /* >> * If GSSAPI is enabled, we need a credential cache; we may >> * already have it, or set it up if not. Then, if we don't >> * have a GSS context, request it and switch to >> * CONNECTION_GSS_STARTUP to wait for the response. >> * >> * Fail altogether if GSS is required but cannot be had. >> */ > > We don't set up a credential cache at any point in this code, we only > check to see if one exists, and only in that case do we send a request > to start GSSAPI encryption (if it's allowed for us to do so). > > Maybe part of the confusion here is that there's two different things- a > credential cache, and then a credential *handle*. Calling > gss_acquire_cred() will, if a credential *cache* exists, return to us a > credential *handle* (in the form of conn->gcred) that we then pass to > gss_init_sec_context(). This is true, though we tend to play fast and loose with that distinction and I'm guilty of doing so here. > There's then also a GSS *context* (conn->gctx), which gets set up when > we first call gss_init_sec_context(), and is then used throughout a > connection. > > Typically, the credential cache is actually created when you log into a > kerberized system, but if not, you can create one by using 'kinit' > manually. > > Hopefully that helps. I'm certainly happy to work with you to reword > the comment, of course, but let's make sure there's agreement and > understanding of what the code does first. How do you feel about something like this: If GSSAPI encryption
Re: Greatest Common Divisor
Bonsoir Vik,
+int4gcd_internal(int32 arg1, int32 arg2)
+{
+ int32 swap;
+
+ /*
+* Put the greater value in arg1.
+* This would happen automatically in the loop below, but avoids an
+* expensive modulo simulation on some architectures.
+*/
+ if (arg1 < arg2)
+ {
+ swap = arg1;
+ arg1 = arg2;
+ arg2 = swap;
+ }
The point of swapping is to a void possibly expensive modulo, but this
should be done on absolute values, otherwise it may not achieve its
purpose as stated by the comment?
gcd() is now strictly positive, so INT_MIN is no longer a valid result.
Ok.
I'm unsure about gcd(INT_MIN, 0) should error. Possibly 0 would be
nicer?
--
Fabien.
Re: Greatest Common Divisor
On Fri, Jan 3, 2020 at 1:57 PM Chapman Flack wrote: > Is there a middle ground staring us in the face, where certain things > could be added in core, but in a new schema like pg_math (pg_ !), so > if you want them you put them on your search path or qualify them > explicitly, and if you don't, you don't? I guess, but it seems like a patch whose mandate is to add one or two functions should not be burdened with inventing an entirely new way to do extensibility. Also, I'm not entirely sure that really addresses all the concerns. Part of my concern about continually adding new functions to core comes from the fact that it bloats the core code, and moving things to another schema does not help with that. It does potentially help with the namespace pollution issue, but how much of an issue is that anyway? Unless you've set up an unusual search_path configuration, your own schemas probably precede pg_catalog in your search path, besides which it seems unlikely that many people have a gcd() function that does anything other than take the greatest common divisor. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Greatest Common Divisor
On 1/3/20 1:46 PM, Robert Haas wrote: > On Fri, Jan 3, 2020 at 1:10 PM Merlin Moncure wrote: >> Just stop doing it. It's very little extra work to package an item >> into an extension and this protects your hapless users who might have >> implemented a function called gcd() that does something different. >> ... > There are counter-arguments to that, though. Maintaining a lot of > extensions with only one or two functions in them is a nuisance. > Having things installed by default is convenient for wanting to use > them. Maintaining contrib code so that it works whether or not the SQL > definitions have been updated via ALTER EXTENSION .. UPDATE takes some > work and thought, and sometimes we screw it up. Is there a middle ground staring us in the face, where certain things could be added in core, but in a new schema like pg_math (pg_ !), so if you want them you put them on your search path or qualify them explicitly, and if you don't, you don't? Regards, -Chap
Re: Assigning ROW variable having NULL value to RECORD type variable doesn't give any structure to the RECORD variable.
On Wed, Jan 1, 2020 at 10:50 AM Ashutosh Sharma wrote: > I know this is expected to happen considering the changes done in > above commit because from this commit onwards, NULL value assigned to > any row variable represents a true NULL composite value before this > commit it used to be a tuple with each column having null value in it. > But, the point is, even if the row variable is having a NULL value it > still has a structure associated with it. Shouldn't that structure be > transferred to RECORD variable when it is assigned with a ROW type > variable ? Can we consider this behaviour change as a side effect of > the improvement done in the RECORD type of variable? I'm not an expert on this topic. However, I *think* that you're trying to distinguish between two things that are actually the same. If it's a "true NULL," it has no structure; it's just NULL. If it has a structure, then it's really a composite value with a NULL in each defined column, i.e. (NULL, NULL, NULL, ...) for some row type rather than just NULL. I have to admit that I've always found PL/pgsql to be a bit pedantic about this whole thing. For instance: rhaas=# do $$declare x record; begin raise notice '%', x.a; end;$$ language plpgsql; ERROR: record "x" is not assigned yet DETAIL: The tuple structure of a not-yet-assigned record is indeterminate. CONTEXT: SQL statement "SELECT x.a" PL/pgSQL function inline_code_block line 1 at RAISE But maybe it should just make x.a evaluate to NULL. It's one thing if I have a record with columns 'a' and 'b' and I ask for column 'c'; I guess you could call that NULL, but it feels reasonably likely to be a programming error. But if we have no idea what the record columns are at all, perhaps we could just assume that whatever column the user is requesting is intended to be one of them, and that since the whole thing is null, that column in particular is null. On the other hand, maybe that would be too lenient and lead to subtle and hard-to-find bugs in plpgsql programs. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH] Increase the maximum value track_activity_query_size
Robert Haas writes: > I vote for not trying to make this more complicated and just accepting > the original proposal. It's about a factor of ten increase over the > limit we have right now, which doesn't seem like enough to cause any > real breakage, and it should be enough to satisfy the majority of the > people who are unhappy with the current limit, and it is very little > work. +1 ... we've surely beaten this topic to death by now. regards, tom lane
Re: Greatest Common Divisor
On Fri, Jan 3, 2020 at 1:10 PM Merlin Moncure wrote: > Just stop doing it. It's very little extra work to package an item > into an extension and this protects your hapless users who might have > implemented a function called gcd() that does something different. > Ideally, the public namespace should contain (by default) only sql > standard functions with all non-standard material in an appropriate > extension. Already released material is obviously problematic and > needs more thought but we ought to at least stop making the problem > worse if possible. There are counter-arguments to that, though. Maintaining a lot of extensions with only one or two functions in them is a nuisance. Having things installed by default is convenient for wanting to use them. Maintaining contrib code so that it works whether or not the SQL definitions have been updated via ALTER EXTENSION .. UPDATE takes some work and thought, and sometimes we screw it up. I don't find any position on this topic to be without merit. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH] Increase the maximum value track_activity_query_size
On Thu, Jan 2, 2020 at 3:27 PM Jeff Janes wrote: > I've seen some pretty big IN-lists and VALUES lists. They aren't so hard to > debug once you tune out iterations 3 through N-3 of the list members. Unless > they are hard to debug for other reasons. In these cases, it would be > helpful, if not just allowing bigger texts in general, to instead "truncate" > from the middle, preserving both the beginning and the end of the query text. I vote for not trying to make this more complicated and just accepting the original proposal. It's about a factor of ten increase over the limit we have right now, which doesn't seem like enough to cause any real breakage, and it should be enough to satisfy the majority of the people who are unhappy with the current limit, and it is very little work. If somebody wants to do more work on this later, they can, but I don't think the OP should be on the hook for that. At some point, someone (I think Peter Geoghegan) suggested that pg_stat_statements ought to normalize IN lists down to a single element. That kind of thing might be another approach to the problem you mention. It's a bit easier to imagine doing such a thing from a tool like that than it is to do it for strings in pg_stat_activity because pg_stat_statements has got a parse tree to work with, not just a flat sting. And that might work more nicely than just keeping the beginning and end of the string, but of course it's also more complicated, so I don't know. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Greatest Common Divisor
On 02/01/2020 16:12, Tom Lane wrote:
> Stephen Frost writes:
>> * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
>>> I'm not objecting to adding it, I'm just curious. In fact, I think
>>> that if we do add this, then we should probably add lcm() at the same
>>> time, since handling its overflow cases is sufficiently non-trivial to
>>> justify not requiring users to have to implement it themselves.
>> I tend to agree with this.
> Does this impact the decision about whether we need a variant for
> numeric? I was leaning against that, primarily because (a)
> it'd introduce a set of questions about what to do with non-integral
> inputs, and (b) it'd make the patch quite a lot larger, I imagine.
> But a variant of lcm() that returns numeric would have much more
> resistance to overflow.
>
> Maybe we could just define "lcm(bigint, bigint) returns numeric"
> and figure that that covers all cases, but it feels slightly
> weird. You couldn't do lcm(lcm(a,b),c) without casting.
> I guess that particular use-case could be addressed with
> "lcm(variadic bigint[]) returns numeric", but that's getting
> really odd.
Okay. Here is a version that should handle everyone's comments.
gcd() is now strictly positive, so INT_MIN is no longer a valid result.
I added an lcm() function. It returns the same type as its arguments so
overflow is possible. I made this choice because int2mul returns int2
(and same for its friends). One can just cast to a bigger integer if
needed.
Because of that, I added a version of gcd() and lcm() for numeric. This
was my first time working with numeric so reviewers should pay extra
attention there, please.
Patch attached.
--
Vik Fearing
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 57a1539506..e2b7d6d240 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -870,6 +870,32 @@
-43
+
+
+
+ gcd
+
+gcd(a, b)
+
+ (same as argument types)
+ greatest common divisor
+ gcd(1071, 462)
+ 21
+
+
+
+
+
+ lcm
+
+lcm(a, b)
+
+ (same as argument types)
+ least common multiple
+ lcm(1071, 462)
+ 23562
+
+
diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index 583ce71e66..a70009ae09 100644
--- a/src/backend/utils/adt/int.c
+++ b/src/backend/utils/adt/int.c
@@ -1196,6 +1196,199 @@ int2abs(PG_FUNCTION_ARGS)
PG_RETURN_INT16(result);
}
+/*
+ * Greatest Common Divisor
+ *
+ * Special cases:
+ * - gcd(x, 0) = x; because 0 is divisible by anything
+ * - gcd(0, 0) = 0; complies with the previous definition and is a
+ *common convention
+ *
+ * The following cases involving INT_MIN have two possible results. They could
+ * return INT_MIN because INT_MIN is a valid divisor of INT_MIN, or they could
+ * throw an exception because the result is negative.
+ * The consensus is to throw an exception.
+ *
+ * - gcd(INT_MIN, 0)
+ * - gcd(INT_MIN, INT_MIN)
+ *
+ * Any other value with INT_MIN will be a positive value representable within
+ * the data type.
+ */
+static int32
+int4gcd_internal(int32 arg1, int32 arg2)
+{
+ int32 swap;
+
+ /*
+ * Put the greater value in arg1.
+ * This would happen automatically in the loop below, but avoids an
+ * expensive modulo simulation on some architectures.
+ */
+ if (arg1 < arg2)
+ {
+ swap = arg1;
+ arg1 = arg2;
+ arg2 = swap;
+ }
+
+ /* Special care needs to be taken with INT_MIN. See comments above. */
+ if (arg2 == PG_INT32_MIN)
+ {
+ if (arg1 == 0 || arg1 == PG_INT32_MIN)
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("integer out of range")));
+ arg1 = -arg1;
+ }
+
+ /* Find GCD using the basic Euclidean algorithm */
+ while (arg2 != 0)
+ {
+ swap = arg2;
+ arg2 = arg1 % arg2;
+ arg1 = swap;
+ }
+
+ /*
+ * Make sure the result is positive. (We know we don't have INT_MIN
+ * anymore).
+ */
+ if (arg1 < 0)
+ arg1 = -arg1;
+
+ return arg1;
+}
+
+static int16
+int2gcd_internal(int16 arg1, int16 arg2)
+{
+ /* See int4gcd_internal for commented version. */
+ int16 swap;
+
+ if (arg1 < arg2)
+ {
+ swap = arg1;
+ arg1 = arg2;
+ arg2 = swap;
+ }
+
+ if (arg2 == PG_INT16_MIN)
+ {
+ if (arg1 == 0 || arg1 == PG_INT16_MIN)
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("smallint out of range")));
+ arg1 = -arg1;
+ }
+
+ while (arg2 != 0)
+ {
+ swap = arg2;
+ arg2 = arg1 % arg2;
+ arg1 = swap;
+ }
+
+ if (arg1 < 0)
+ arg1 = -arg1;
+
+ return arg1;
+}
+
+Datum
+int4gcd(PG_FUNCTION_ARGS)
+{
+ int32 arg1 = PG_GETARG_INT32(0);
+ int32 arg2 = PG_GETARG_INT32(1);
+ int32 result;
+
+ result = int4gcd_internal(arg1, arg2);
+
+ PG_RETURN_INT32(result);
+}
+
+Datum
+int2gcd(PG_FUNCTION_ARGS)
+{
+ int16 arg1 = PG_GETARG_INT16(0);
+ int16 arg2 = PG_GETARG_INT16(1);
+ int16 result;
+
+ result =
Re: \d is not showing global(normal) table info if we create temporary table with same name as global table
On Fri, 3 Jan 2020 at 00:40, Tom Lane wrote: > > Robert Haas writes: > > On Thu, Jan 2, 2020 at 12:59 PM Mahendra Singh wrote: > >> While reading code and doing some testing, I found that if we create a > >> temporary table with same name as we created a normal(global) table, then > >> \d is showing only temporary table info. > > > That's because the query that \d issues to the backend includes: > > AND pg_catalog.pg_table_is_visible(c.oid) > > So I'd say it's not a bug, because that bit of SQL didn't get included > > in the query by accident. > > It's also documented: > > Whenever the pattern parameter is omitted completely, the \d commands > display all objects that are visible in the current schema search path > — this is equivalent to using * as the pattern. (An object is said to > be visible if its containing schema is in the search path and no > object of the same kind and name appears earlier in the search > path. This is equivalent to the statement that the object can be > referenced by name without explicit schema qualification.) To see all > objects in the database regardless of visibility, use *.* as the > pattern. > > Perhaps that's not clear enough, but the behavior is certainly as-intended. > > regards, tom lane Thanks Robert and Tom for quick detailed response. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Christoph Berg writes: > Re: Tom Lane 2020-01-03 <26339.1578072...@sss.pgh.pa.us> >> You do? I went looking in the Debian package source repo just the >> other day for some evidence that that was true, and couldn't find >> any, so I concluded that it was only an urban legend. Where is that >> done exactly? > /usr/share/postgresql-common/pg_wrapper > https://salsa.debian.org/postgresql/postgresql-common/blob/master/pg_wrapper#L129-157 Oh, so not in the Postgres package per se. What that means is that our regression tests will pass (as it's just a regular libedit install while they're running) but then filename completion will not work well for actual users. And there's not really anything I can do about that from this end. (On the other hand, filename completion is already kind of buggy, which is why that patch exists in the first place. So maybe it won't get any worse. Hard to say.) regards, tom lane
Re: color by default
On Thu, Jan 2, 2020 at 6:38 PM Gavin Flower wrote: > I find coloured output very difficult to read, as the colours seem to be > chosen on the basis everyone uses white as the background colour for > terminals -- whereas I use black, as do a lot of other people. I don't like colored output either. (It is, however, probably not a surprise to anyone that I am old-school in many regards, so how much my opinion ought to count is debatable. I still use \pset linestyle old-ascii when I remember to set it, use vi to edit, with hjkl rather than arrow keys, and almost always prefer a CLI to a GUI when I have the option. I have conceded the utility of indoor heat and plumbing, though, so maybe there's hope for me yet.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Greatest Common Divisor
On Fri, Jan 3, 2020 at 10:24 AM Robert Haas wrote: > > On Fri, Jan 3, 2020 at 10:23 AM Tom Lane wrote: > > Now, those functions were just exposing libc functionality, so there > > wasn't a lot of code to write. There might be a good argument that > > gcd isn't useful enough to justify the amount of code we'd have to > > add (especially if we allow it to scope-creep into needing to deal > > with "numeric" calculations). But I'm not on board with just > > dismissing it as uninteresting. > > Yeah. There's always the question with things like this as to whether > we ought to push certain things into contrib modules that are not > installed by default to avoid bloating the set of things built into > the core server. But it's hard to know where to draw the line. Just stop doing it. It's very little extra work to package an item into an extension and this protects your hapless users who might have implemented a function called gcd() that does something different. Ideally, the public namespace should contain (by default) only sql standard functions with all non-standard material in an appropriate extension. Already released material is obviously problematic and needs more thought but we ought to at least stop making the problem worse if possible. merlin
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
On Fri, Jan 3, 2020 at 12:48 PM Christoph Berg wrote: > > Perhaps more importantly, *why* is it done? It seems to me that it > > takes a pretty fevered imagination to suppose that using libreadline > > Tom, claiming that things are "fevered" just because you didn't like > them is not appropriate. It's not fun working with PostgreSQL when the > tone is like that. +1. > > that way meets the terms of its license but just building against > > the library normally would not. Certainly when I worked for Red Hat, > > their lawyers did not think there was any problem with building > > Postgres using both openssl and readline. > > I'm not starting that debate here, but Debian thinks otherwise: > > https://lwn.net/Articles/428111/ I take no position on whether Debian is correct in its assessment of such things, but I reiterate my previous opposition to breaking it just because we don't agree with it, or because Tom specifically doesn't. It's too mainstream a platform to arbitrarily break. And it will probably just have the effect of increasing the number of patches they're carrying against our sources, which will not make things better for anybody. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes: > Tom Lane writes: >> Anybody know an easy way to do that in Perl? > I was going to suggest using Test::More's like() function to do the > regex check, but sadly that only escapes things that would break the TAP > stream syntax, not non-printables in general. The next obvious thing is > Data::Dumper with the 'Useqq' option enabled, which makes it use > double-quoted-string escapes (e.g. "\a" for ^G). > The attaced patch does that, and also bumps $Test::Builder::Level so the > diagnostic references the calling line, and uses diag() instad of > note(), so it shows even in non-verbose mode. LGTM, pushed (along with a fix to deal with what hopefully is the only remaining obstacle for Andres' critters). regards, tom lane
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Re: Tom Lane 2020-01-03 <26339.1578072...@sss.pgh.pa.us> > Christoph Berg writes: > > Re: Tom Lane 2020-01-03 <13708.1578059...@sss.pgh.pa.us> > >> I found out while investigating this that the libedit version shipping > >> with buster (3.1-20181209) is differently broken for the same case: > > > (Fwiw this wasn't spotted before because we have this LD_PRELOAD hack > > that replaces libedit with readline at psql runtime. > > You do? I went looking in the Debian package source repo just the > other day for some evidence that that was true, and couldn't find > any, so I concluded that it was only an urban legend. Where is that > done exactly? /usr/share/postgresql-common/pg_wrapper https://salsa.debian.org/postgresql/postgresql-common/blob/master/pg_wrapper#L129-157 > Perhaps more importantly, *why* is it done? It seems to me that it > takes a pretty fevered imagination to suppose that using libreadline Tom, claiming that things are "fevered" just because you didn't like them is not appropriate. It's not fun working with PostgreSQL when the tone is like that. > that way meets the terms of its license but just building against > the library normally would not. Certainly when I worked for Red Hat, > their lawyers did not think there was any problem with building > Postgres using both openssl and readline. I'm not starting that debate here, but Debian thinks otherwise: https://lwn.net/Articles/428111/ > The reason I'm concerned about this is that there's a patch on the > table [1] that will probably not behave nicely at all if it's > compiled against libedit headers and then executed with libreadline, > because it will draw the wrong conclusions about whether the > filename quoting hooks are available. So that hack is going to > fail on you soon, especially after I add regression testing around > the filename completion stuff ;-) Well, so far, it worked well. (The biggest problem used to be that libedit didn't have the history append function so it wasn't used even with readline, but that got implemented ~2 years ago.) Christoph
Re: accounting for memory used for BufFile during hash joins
On Mon, Nov 25, 2019 at 10:11 AM Tomas Vondra wrote: > On Mon, Nov 25, 2019 at 05:33:35PM +0900, Michael Paquier wrote: > >On Tue, Sep 10, 2019 at 03:47:51PM +0200, Tomas Vondra wrote: > >> My feeling is that we should get the BNLJ committed first, and then > maybe > >> use some of those additional strategies as fallbacks (depending on which > >> issues are still unsolved by the BNLJ). > > > >The glacier is melting more. Tomas, what's your status here? The > >patch has been waiting on author for two months now. If you are not > >planning to work more on this one, then it should be marked as > >returned with feedback? > > I'm not planning to do any any immediate work on this, so I agree with > marking it as RWF. I think Melanie is working on the BNL patch, which > seems like the right solution. > > Sorry for the delay. I have posted the parallel-aware version BNLJ (adaptive HJ) of this in the thread which originally had all of the patches for it [1]. It's not near committable, so I wasn't going to register it for a commitfest yet, but I would love feedback on the prototype. [1] https://www.postgresql.org/message-id/CAAKRu_YsWm7gc_b2nBGWFPE6wuhdOLfc1LBZ786DUzaCPUDXCA%40mail.gmail.com -- Melanie Plageman
Re: backup manifests
On Fri, Jan 3, 2020 at 12:01 PM Stephen Frost wrote: > You're certainly intending to do *something* with the manifest, and > while I appreciate that you feel you've come up with a complete use-case > that this simple manifest will be sufficient for, I frankly doubt > that'll actually be the case. Not long ago it wasn't completely clear > that a manifest at *all* was even going to be necessary for the specific > use-case you had in mind (I'll admit I wasn't 100% sure myself at the > time either), but now that we're down the road of having one, I can't > agree with the blanket assumption that we're never going to want to > extend it, or even that it won't be necessary to add to it before this > particular use-case is fully addressed. > > And the same goes for the other things that were discussed up-thread > regarding memory context and error handling and such. Well, I don't know how to make you happy here. It looks to me like insisting on a JSON-format manifest will likely mean that this doesn't get into PG13 or PG14 or probably PG15, because a port of all that machinery to work in frontend code will be neither simple nor quick. If you want this to happen for this release, you've got to be willing to settle for something that can be implemented in the time we have. I'm not sure whether what you and David are arguing boils down to thinking that I'm wrong when I say that doing that is hard, or whether you know it's hard but you just don't care because you'd rather see the feature go nowhere than use a format other than JSON. I don't see much difference between the latter position and a desire to block the feature permanently. And if it's the former then you have yet to make any suggestions for how to get it done with reasonable effort. > I'm happy to outline the other things that one *might* want to include > in a manifest, if that would be helpful, but I'll also say that I'm not > planning to hack on adding that to pg_basebackup in the next month or > two. Once we've actually got a manifest, if it's in an extendable > format, I could certainly see people wanting to do more with it though. Well, as I say, it's got a version number, so somebody can always come along with something better. I really think this is a red herring, though. If somebody wants to track additional data about a backup, there's no rule that they have to include it in the backup manifest. A backup management solution might want to track things like who initiated the backup, or for what purpose it was taken, or the IP address of the machine where it was taken, or the backup system's own identifier, but any of that stuff could (and probably should) be stored in a file managed by that tool rather than in the server's own manifest. As to the per-file information, I believe that David and I discussed that and the list of fields that I had seemed relatively OK, and I believe I added at least one (mtime) per his suggestion. Of course, it's a tab-separated file; more fields could easily be added at the end, separated by tabs. Or, you could modify the file so that after each "File" line you had another line with supplementary information about that file, beginning with some other word. Or, you could convert the whole file to JSON for v2 of the manifest, if, contrary to my belief, that's a fairly simple thing to do. There are probably other approaches as well. This file format has already had considerably more thought about forward-compatibility than pg_hba.conf, which has been retrofitted multiple times without breaking the world. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Greatest Common Divisor
On 2020-Jan-03, Robert Haas wrote: > On Fri, Jan 3, 2020 at 10:23 AM Tom Lane wrote: > > Now, those functions were just exposing libc functionality, so there > > wasn't a lot of code to write. There might be a good argument that > > gcd isn't useful enough to justify the amount of code we'd have to > > add (especially if we allow it to scope-creep into needing to deal > > with "numeric" calculations). But I'm not on board with just > > dismissing it as uninteresting. > > Yeah. There's always the question with things like this as to whether > we ought to push certain things into contrib modules that are not > installed by default to avoid bloating the set of things built into > the core server. But it's hard to know where to draw the line. There's > no objective answer to the question of whether gcd() or sinh() is more > useful to have in core; The SQL standard's feature T622 requires trigonometric functions, while it doesn't list gcd() or anything of the sort, so there's that. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Christoph Berg writes: > Re: Tom Lane 2020-01-03 <13708.1578059...@sss.pgh.pa.us> >> I found out while investigating this that the libedit version shipping >> with buster (3.1-20181209) is differently broken for the same case: > (Fwiw this wasn't spotted before because we have this LD_PRELOAD hack > that replaces libedit with readline at psql runtime. You do? I went looking in the Debian package source repo just the other day for some evidence that that was true, and couldn't find any, so I concluded that it was only an urban legend. Where is that done exactly? Perhaps more importantly, *why* is it done? It seems to me that it takes a pretty fevered imagination to suppose that using libreadline that way meets the terms of its license but just building against the library normally would not. Certainly when I worked for Red Hat, their lawyers did not think there was any problem with building Postgres using both openssl and readline. The reason I'm concerned about this is that there's a patch on the table [1] that will probably not behave nicely at all if it's compiled against libedit headers and then executed with libreadline, because it will draw the wrong conclusions about whether the filename quoting hooks are available. So that hack is going to fail on you soon, especially after I add regression testing around the filename completion stuff ;-) >> I used a "note" command to print it, maybe that's not best practice? > I think best practice is to use something like > like($out, qr/$pattern/, $annotation) I'll check into that, thanks! regards, tom lane [1] https://www.postgresql.org/message-id/flat/16059-8836946734c02...@postgresql.org
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Tom Lane writes:
> Also, while I'm asking for Perl advice: I can see in my editor that
> there's a control-G bell character in that string, but this is far
> from obvious on the web page. I'd kind of like to get the report
> to escapify control characters so that what comes out is more like
>
> # Actual output was "\DRD^G"
> or
> # Actual output was "\\DRD\007"
>
> or some such. Anybody know an easy way to do that in Perl?
I was going to suggest using Test::More's like() function to do the
regex check, but sadly that only escapes things that would break the TAP
stream syntax, not non-printables in general. The next obvious thing is
Data::Dumper with the 'Useqq' option enabled, which makes it use
double-quoted-string escapes (e.g. "\a" for ^G).
The attaced patch does that, and also bumps $Test::Builder::Level so the
diagnostic references the calling line, and uses diag() instad of
note(), so it shows even in non-verbose mode.
- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen
>From c4541abc826c40e88f729c5a71e5d06a11295aa2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?=
Date: Fri, 3 Jan 2020 17:07:10 +
Subject: [PATCH] Escape non-printable characters in psql tab-completion TAP
tests
By using Data::Dumper in Useqq mode.
In passing, bump $Test::Builder::Level so the diagnostic references the
calling line, and use diag() instad of note(), so it shows even in
non-verbose mode.
---
src/bin/psql/t/010_tab_completion.pl | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index a02cbd8e47..553288bda7 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -5,6 +5,7 @@ use PostgresNode;
use TestLib;
use Test::More;
use IPC::Run qw(pump finish timer);
+use Data::Dumper;
if (!defined($ENV{with_readline}) || $ENV{with_readline} ne 'yes')
{
@@ -52,6 +53,9 @@ sub check_completion
{
my ($send, $pattern, $annotation) = @_;
+ # report test failures from caller location
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
# reset output collector
$out = "";
# restart per-command timer
@@ -63,7 +67,9 @@ sub check_completion
my $okay = ($out =~ m/$pattern/ && !$timer->is_expired);
ok($okay, $annotation);
# for debugging, log actual output if it didn't match
- note 'Actual output was "' . $out . "\"\n" if !$okay;
+ local $Data::Dumper::Terse = 1;
+ local $Data::Dumper::Useqq = 1;
+ diag 'Actual output was ' . Dumper($out) . "\n" if !$okay;
return;
}
--
2.22.0
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Re: Tom Lane 2020-01-03 <13708.1578059...@sss.pgh.pa.us>
> I found out while investigating this that the libedit version shipping
> with buster (3.1-20181209) is differently broken for the same case:
(Fwiw this wasn't spotted before because we have this LD_PRELOAD hack
that replaces libedit with readline at psql runtime. I guess that
means that the hack is pretty stable... Still, looking forward to the
day that OpenSSL is finally relicensing so we can properly link to
readline.)
Re: Tom Lane 2020-01-03 <14261.1578060...@sss.pgh.pa.us>
> > Shouldn't this print some "expected foo, got bar" diagnostics instead
> > of just dying?
>
> BTW, as far as that goes, we do: see for instance the tail end of
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=desmoxytes=2020-01-02%2020%3A04%3A03
>
> ok 8 - offer multiple table choices
> ok 9 - finish completion of one of multiple table choices
> ok 10 - \r works
> not ok 11 - complete \DRD to \drds
>
> # Failed test 'complete \DRD to \drds'
> # at t/010_tab_completion.pl line 64.
> # Actual output was "\DRD"
> ok 12 - \r works
>
> Not sure why you are not seeing the "Actual output" bit in your log.
> I used a "note" command to print it, maybe that's not best practice?
I think best practice is to use something like
like($out, qr/$pattern/, $annotation)
instead of plain "ok()" which doesn't know about the actual values
compared. The "&& !$timer->is_expired" condition can be dropped from
the test because all we care about is if the output matches.
I never really grasped in which contexts TAP is supposed to print the
full test output ("ok 10 -..."). Apparently the way the testsuite is
invoked at package build time only prints the terse failure summary in
which "note"s aren't included. Is there a switch to configure that?
> Also, while I'm asking for Perl advice: I can see in my editor that
> there's a control-G bell character in that string, but this is far
> from obvious on the web page. I'd kind of like to get the report
> to escapify control characters so that what comes out is more like
>
> # Actual output was "\DRD^G"
> or
> # Actual output was "\\DRD\007"
>
> or some such. Anybody know an easy way to do that in Perl?
I don't know for note(), but maybe like() would do that automatically.
Christoph
Re: backup manifests
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Fri, Jan 3, 2020 at 11:44 AM Stephen Frost wrote: > > Sure, it'd be work, and for "adding a simple backup manifest", maybe too > > much to be worth considering ... but that's not what is going on here, > > is it? Are we really *just* going to add a backup manifest to > > pg_basebackup and call it done? That's not what I understood the goal > > here to be but rather to start doing a lot of other things with > > pg_basebackup beyond just having a manifest and if you think just a bit > > farther down the path, I think you start to realize that you're going to > > need this base set of capabilities to get to a point where pg_basebackup > > (or whatever it ends up being called) is able to have the kind of > > capabilities that exist in other PG backup software already. > > I have no development plans for pg_basebackup that require extending > the format of the manifest file in any significant way, and am not > aware that anyone else has such plans either. If you are aware of > something I'm not, or if anyone else is, it would be helpful to know > about it. You're certainly intending to do *something* with the manifest, and while I appreciate that you feel you've come up with a complete use-case that this simple manifest will be sufficient for, I frankly doubt that'll actually be the case. Not long ago it wasn't completely clear that a manifest at *all* was even going to be necessary for the specific use-case you had in mind (I'll admit I wasn't 100% sure myself at the time either), but now that we're down the road of having one, I can't agree with the blanket assumption that we're never going to want to extend it, or even that it won't be necessary to add to it before this particular use-case is fully addressed. And the same goes for the other things that were discussed up-thread regarding memory context and error handling and such. I'm happy to outline the other things that one *might* want to include in a manifest, if that would be helpful, but I'll also say that I'm not planning to hack on adding that to pg_basebackup in the next month or two. Once we've actually got a manifest, if it's in an extendable format, I could certainly see people wanting to do more with it though. Thanks, Stephen signature.asc Description: PGP signature
Re: sidewinder has one failure
I wrote: > Amit Kapila writes: >> The problem we are seeing on this machine is that I think we have >> seven files opened before we reach function set_max_safe_fds during >> startup. Now, it is not clear to me why it is opening extra file(s) >> during start-up as compare to other machines. > Maybe it uses one of the semaphore implementations that consume a > file descriptor per semaphore? Hm, no, sidewinder reports that it's using SysV semaphores: checking which semaphore API to use... System V However, I tried building an installation that uses named POSIX semaphores, by applying the attached hack on a macOS system. And sure enough, this test crashes and burns: 2020-01-03 11:36:21.571 EST [91597] FATAL: insufficient file descriptors available to start server process 2020-01-03 11:36:21.571 EST [91597] DETAIL: System allows -8, we need at least 20. 2020-01-03 11:36:21.571 EST [91597] LOG: database system is shut down Looking at "lsof" output for a postmaster with max_connections=10, max_wal_senders=5 (the parameters set up by PostgresNode.pm), I see that it's got 31 "PSXSEM" file descriptors, so the number shown here is about what you'd expect. We might be able to constrain that down a little further, but still, this test has no chance of working in anything like its present form on a machine that needs file descriptors for semaphores. That's a supported configuration, even if not a recommended one, so I don't think it's okay for the test to fall over. (Hmm ... apparently, we have no buildfarm members that use such semaphores and are running the TAP tests, else we'd have additional complaints. Perhaps that's a bad omission.) Anyway, it remains unclear exactly why sidewinder is failing, but I'm guessing it has a few more open files than you expected. My macOS build has a few more than I can account for in my caffeine- deprived state, too. One of them might be for bonjour ... not sure about some of the rest. Bottom line here is that it's hard to predict with any accuracy how many pre-opened files there will be. regards, tom lane diff --git a/src/template/darwin b/src/template/darwin index f4d4e9d7cf..98331be22d 100644 --- a/src/template/darwin +++ b/src/template/darwin @@ -23,11 +23,4 @@ CFLAGS_SL="" # support System V semaphores; before that we have to use named POSIX # semaphores, which are less good for our purposes because they eat a # file descriptor per backend per max_connection slot. -case $host_os in - darwin[015].*) USE_NAMED_POSIX_SEMAPHORES=1 -;; - *) -USE_SYSV_SEMAPHORES=1 -;; -esac
Re: backup manifests
On Fri, Jan 3, 2020 at 11:44 AM Stephen Frost wrote: > Sure, it'd be work, and for "adding a simple backup manifest", maybe too > much to be worth considering ... but that's not what is going on here, > is it? Are we really *just* going to add a backup manifest to > pg_basebackup and call it done? That's not what I understood the goal > here to be but rather to start doing a lot of other things with > pg_basebackup beyond just having a manifest and if you think just a bit > farther down the path, I think you start to realize that you're going to > need this base set of capabilities to get to a point where pg_basebackup > (or whatever it ends up being called) is able to have the kind of > capabilities that exist in other PG backup software already. I have no development plans for pg_basebackup that require extending the format of the manifest file in any significant way, and am not aware that anyone else has such plans either. If you are aware of something I'm not, or if anyone else is, it would be helpful to know about it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] Block level parallel vacuum
On Sun, Dec 29, 2019 at 4:23 PM Tomas Vondra wrote: > IMO there's not much reason for the leader not to participate. For > regular queries the leader may be doing useful stuff (essentially > running the non-parallel part of the query) but AFAIK for VAUCUM that's > not the case and the worker is not doing anything. I agree, and said the same thing in http://postgr.es/m/CA+Tgmob7JLrngeHz6i60_TqdvE1YBcvGYVoEQ6_xvP=vn7d...@mail.gmail.com I really don't know why we have that code. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: backup manifests
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Robert Haas writes: > > AFAICS, the only options to make that work with JSON are (1) introduce > > a new hand-coded JSON parser designed for frontend operation, (2) add > > a dependency on an external JSON parser that we can use from frontend > > code, or (3) adapt the existing JSON parser used in the backend so > > that it can also be used in the frontend. > > ... I'd > > be willing to do (3) if somebody could explain to me how to solve the > > problems with porting that code to work on the frontend side, but the > > only suggestion so far as to how to do that is to port memory > > contexts, elog/report, and presumably encoding handling to work on the > > frontend side. That seems to me to be an unreasonably large lift, > > Yeah, agreed. The only consideration that'd make that a remotely > sane idea is that if somebody did the work, there would be other > uses for it. (One that comes to mind immediately is cleaning up > ecpg's miserably-maintained fork of the backend datetime code.) > > But there's no denying that it would be a large amount of work > (if it's even feasible), and nobody has stepped up to volunteer. > It's not reasonable to hold up this particular feature waiting > for that to happen. Sure, it'd be work, and for "adding a simple backup manifest", maybe too much to be worth considering ... but that's not what is going on here, is it? Are we really *just* going to add a backup manifest to pg_basebackup and call it done? That's not what I understood the goal here to be but rather to start doing a lot of other things with pg_basebackup beyond just having a manifest and if you think just a bit farther down the path, I think you start to realize that you're going to need this base set of capabilities to get to a point where pg_basebackup (or whatever it ends up being called) is able to have the kind of capabilities that exist in other PG backup software already. I'm sure I don't need to say where to find it, but I can point you to a pretty good example of a similar effort, and we didn't start with "build a manifest into a custom format" as the first thing implemented, but rather a great deal of work was first put into building out things like logging, memory management/contexts, error handling/try-catch, having a string type, a variant type, etc. In some ways, it's kind of impressive what we've got in our front-ends tools even though we don't have these things, really, and certainly not all in one nice library that they all use... but at the same time, I think that lack has also held those tools back, pg_basebackup among them. Anyway, off my high horse, I'll just say I agree w/ David and David wrt using JSON for this over hacking together yet another format. We didn't do that as thoroughly as we should have (we've got a JSON parser and all that, and use JSON quite a bit, but the actual manifest format is a mix of ini-style and JSON, because it's got more in it than just a list of files, something that I suspect will also end up being true of this down the road and for good reasons, and we started with the ini format and discovered it sucked and then started embedding JSON in it...), and we've come to realize that was a bad idea, and intend to fix it in our next manifest major version bump. Would be unfortunate to see PG making that same mistake. Thanks, Stephen signature.asc Description: PGP signature
Re: Greatest Common Divisor
On Fri, Jan 3, 2020 at 10:23 AM Tom Lane wrote: > Now, those functions were just exposing libc functionality, so there > wasn't a lot of code to write. There might be a good argument that > gcd isn't useful enough to justify the amount of code we'd have to > add (especially if we allow it to scope-creep into needing to deal > with "numeric" calculations). But I'm not on board with just > dismissing it as uninteresting. Yeah. There's always the question with things like this as to whether we ought to push certain things into contrib modules that are not installed by default to avoid bloating the set of things built into the core server. But it's hard to know where to draw the line. There's no objective answer to the question of whether gcd() or sinh() is more useful to have in core; each is more useful to people who need that one but not the other, and trying to guess whether more or fewer people need gcd() than sinh() seems like a fool's errand. Perhaps in retrospect we would be better off having a 'math' extension where a lot of this stuff lives, and people who want that extension can install it and others need not bother. But, to try to create that now and move things there would break upgrades for an exceedingly marginal benefit. I don't really like the namespace pollution that comes with accepting feature requests like this, but it's hard to argue that it's a serious show-stopper or that the cure is any less bad than the disease. And I'm sure that I'd be much more likely to use gcd() or lcm() in a query than tanh()... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
explain HashAggregate to report bucket and memory stats
On Sun, Feb 17, 2019 at 11:29:56AM -0500, Jeff Janes wrote:
https://www.postgresql.org/message-id/CAMkU%3D1zBJNVo2DGYBgLJqpu8fyjCE_ys%2Bmsr6pOEoiwA7y5jrA%40mail.gmail.com
> What would I find very useful is [...] if the HashAggregate node under
> "explain analyze" would report memory and bucket stats; and if the Aggregate
> node would report...anything.
Find attached my WIP attempt to implement this.
Jeff: can you suggest what details Aggregate should show ?
Justin
>From 5d0afe5d92649f575d9b09ae19b31d2bfd5bfd12 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Wed, 1 Jan 2020 13:09:33 -0600
Subject: [PATCH v1 1/2] refactor: show_hinstrument and avoid showing memory
use if not verbose..
This changes explain analyze at least for Hash(join), but doesn't break
affect regression tests, since they all run explain without analyze, so
nbatch=0, and no stats are shown.
But for future patch to show stats for HashAgg (for which nbatch=1, always), we
want to show buckets in explain analyze, but don't want to show memory, which
is machine-specific.
---
src/backend/commands/explain.c | 73 ++
1 file changed, 45 insertions(+), 28 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 497a3bd..d5eaf15 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -101,6 +101,7 @@ static void show_sortorder_options(StringInfo buf, Node *sortexpr,
static void show_tablesample(TableSampleClause *tsc, PlanState *planstate,
List *ancestors, ExplainState *es);
static void show_sort_info(SortState *sortstate, ExplainState *es);
+static void show_hinstrument(ExplainState *es, HashInstrumentation *h);
static void show_hash_info(HashState *hashstate, ExplainState *es);
static void show_tidbitmap_info(BitmapHeapScanState *planstate,
ExplainState *es);
@@ -2702,43 +2703,59 @@ show_hash_info(HashState *hashstate, ExplainState *es)
}
}
- if (hinstrument.nbatch > 0)
- {
- long spacePeakKb = (hinstrument.space_peak + 1023) / 1024;
+ show_hinstrument(es, );
+}
- if (es->format != EXPLAIN_FORMAT_TEXT)
- {
- ExplainPropertyInteger("Hash Buckets", NULL,
- hinstrument.nbuckets, es);
- ExplainPropertyInteger("Original Hash Buckets", NULL,
- hinstrument.nbuckets_original, es);
- ExplainPropertyInteger("Hash Batches", NULL,
- hinstrument.nbatch, es);
- ExplainPropertyInteger("Original Hash Batches", NULL,
- hinstrument.nbatch_original, es);
- ExplainPropertyInteger("Peak Memory Usage", "kB",
- spacePeakKb, es);
- }
- else if (hinstrument.nbatch_original != hinstrument.nbatch ||
- hinstrument.nbuckets_original != hinstrument.nbuckets)
- {
+/*
+ * Show hash bucket stats and (optionally) memory.
+ */
+static void
+show_hinstrument(ExplainState *es, HashInstrumentation *h)
+{
+ long spacePeakKb = (h->space_peak + 1023) / 1024;
+
+ if (h->nbatch <= 0)
+ return;
+
+ if (es->format != EXPLAIN_FORMAT_TEXT)
+ {
+ ExplainPropertyInteger("Hash Buckets", NULL,
+ h->nbuckets, es);
+ ExplainPropertyInteger("Original Hash Buckets", NULL,
+ h->nbuckets_original, es);
+ ExplainPropertyInteger("Hash Batches", NULL,
+ h->nbatch, es);
+ ExplainPropertyInteger("Original Hash Batches", NULL,
+ h->nbatch_original, es);
+ ExplainPropertyInteger("Peak Memory Usage", "kB",
+ spacePeakKb, es);
+ }
+ else
+ {
+ if (h->nbatch_original != h->nbatch ||
+ h->nbuckets_original != h->nbuckets) {
appendStringInfoSpaces(es->str, es->indent * 2);
appendStringInfo(es->str,
- "Buckets: %d (originally %d) Batches: %d (originally %d) Memory Usage: %ldkB\n",
- hinstrument.nbuckets,
- hinstrument.nbuckets_original,
- hinstrument.nbatch,
- hinstrument.nbatch_original,
- spacePeakKb);
+ "Buckets: %d (originally %d) Batches: %d (originally %d)",
+ h->nbuckets,
+ h->nbuckets_original,
+ h->nbatch,
+ h->nbatch_original);
}
else
{
appendStringInfoSpaces(es->str, es->indent * 2);
appendStringInfo(es->str,
- "Buckets: %d Batches: %d Memory Usage: %ldkB\n",
- hinstrument.nbuckets, hinstrument.nbatch,
- spacePeakKb);
+ "Buckets: %d Batches: %d",
+ h->nbuckets,
+ h->nbatch);
}
+
+ if (es->verbose && es->analyze)
+ appendStringInfo(es->str,
+ " Memory Usage: %ldkB",
+ spacePeakKb);
+ appendStringInfoChar(es->str, '\n');
}
}
--
2.7.4
>From d691e492b619e5cc6a1fcd4134728c1c0852d589 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Tue, 31 Dec 2019 18:49:41 -0600
Subject: [PATCH v1 2/2] explain analyze to show stats from (hash) aggregate..
..as suggested by Jeff Janes
---
src/backend/commands/explain.c | 50 +++--
src/backend/executor/execGrouping.c | 10
src/include/executor/nodeAgg.h | 1 +
src/include/nodes/execnodes.h
Re: Greatest Common Divisor
Peter Eisentraut writes: > On 2020-01-02 15:50, Dean Rasheed wrote: >> Out of curiosity, what was the original use-case for this? > Yeah, I'm wondering, is this useful for any typical analytics or > business application? Otherwise, abstract algebra functionality seems a > bit out of scope. Nobody complained when we added sinh, cosh, tanh, asinh, acosh, atanh last year, so I'm feeling skeptical of claims that gcd should be out of scope. Now, those functions were just exposing libc functionality, so there wasn't a lot of code to write. There might be a good argument that gcd isn't useful enough to justify the amount of code we'd have to add (especially if we allow it to scope-creep into needing to deal with "numeric" calculations). But I'm not on board with just dismissing it as uninteresting. regards, tom lane
Re: Fetching timeline during recovery
Hi,
On Mon, 23 Dec 2019 15:38:16 +0100
Jehan-Guillaume de Rorthais wrote:
[...]
> My idea would be to return a row from pg_stat_get_wal_receiver() as soon as
> a wal receiver has been replicating during the uptime of the standby, no
> matter if there's one currently working or not. If no wal receiver is active,
> the "pid" field would be NULL and the "status" would reports eg. "inactive".
> All other fields would report their last known value as they are kept in
> shared memory WalRcv struct.
Please, find in attachment a patch implementing the above proposal.
Regards,
>From 5641d8c5d46968873d8b8e1d3c1c0de10551741e Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais
Date: Tue, 31 Dec 2019 18:29:13 +0100
Subject: [PATCH] Always expose available stats from wal receiver
Makes admin function pg_stat_get_wal_receiver() return available data
from WalRcv in shared memory, whatever the state of the wal receiver
process.
This allows supervision or HA tools to gather various physical
replication stats even when the wal receiver is stopped. For example,
the latest timeline the wal receiver was receiving before shutting
down.
The behavior of the pg_stat_wal_receiver view has been kept to avoid
regressions: it returns no row when the wal receiver is shut down.
---
src/backend/replication/walreceiver.c | 14 +-
src/test/recovery/t/004_timeline_switch.pl | 12 +++-
2 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index a4de8a9cd8..1207f145b8 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1354,13 +1354,6 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
strlcpy(conninfo, (char *) WalRcv->conninfo, sizeof(conninfo));
SpinLockRelease(>mutex);
- /*
- * No WAL receiver (or not ready yet), just return a tuple with NULL
- * values
- */
- if (pid == 0 || !ready_to_display)
- PG_RETURN_NULL();
-
/* determine result type */
if (get_call_result_type(fcinfo, NULL, ) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
@@ -1369,7 +1362,10 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
nulls = palloc0(sizeof(bool) * tupdesc->natts);
/* Fetch values */
- values[0] = Int32GetDatum(pid);
+ if (pid == 0)
+ nulls[0] = true;
+ else
+ values[0] = Int32GetDatum(pid);
if (!is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS))
{
@@ -1422,7 +1418,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
nulls[12] = true;
else
values[12] = Int32GetDatum(sender_port);
- if (*conninfo == '\0')
+ if (*conninfo == '\0' || !ready_to_display)
nulls[13] = true;
else
values[13] = CStringGetTextDatum(conninfo);
diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index 7e952d3667..cdcdd2d981 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -6,7 +6,7 @@ use warnings;
use File::Path qw(rmtree);
use PostgresNode;
use TestLib;
-use Test::More tests => 2;
+use Test::More tests => 4;
$ENV{PGDATABASE} = 'postgres';
@@ -37,6 +37,11 @@ $node_master->safe_psql('postgres',
$node_master->wait_for_catchup($node_standby_1, 'replay',
$node_master->lsn('write'));
+# Check received timeline from pg_stat_get_wal_receiver() on standby 1
+my $node_standby_1_lsn = $node_standby_1->safe_psql('postgres',
+ 'SELECT received_tli FROM pg_stat_get_wal_receiver()');
+is($node_standby_1_lsn, 1, 'check received timeline on standby 1');
+
# Stop and remove master
$node_master->teardown_node;
@@ -66,3 +71,8 @@ $node_standby_1->wait_for_catchup($node_standby_2, 'replay',
my $result =
$node_standby_2->safe_psql('postgres', "SELECT count(*) FROM tab_int");
is($result, qq(2000), 'check content of standby 2');
+
+# Check received timeline from pg_stat_get_wal_receiver() on standby 2
+my $node_standby_2_lsn = $node_standby_2->safe_psql('postgres',
+ 'SELECT received_tli FROM pg_stat_get_wal_receiver()');
+is($node_standby_2_lsn, 2, 'check received timeline on standby 2');
--
2.20.1
Re: sidewinder has one failure
Amit Kapila writes: > On Fri, Jan 3, 2020 at 6:34 PM Mikael Kjellström > wrote: >> Why is this machine different from everybody else when it comes to this >> limit? > The problem we are seeing on this machine is that I think we have > seven files opened before we reach function set_max_safe_fds during > startup. Now, it is not clear to me why it is opening extra file(s) > during start-up as compare to other machines. Maybe it uses one of the semaphore implementations that consume a file descriptor per semaphore? I think that d20703805 was insanely optimistic to think that a tiny value of max_files_per_process would work the same everywhere. I'd actually recommend just dropping that test, as I do not think it's possible to make it portable and reliable. Even if it could be fixed, I doubt it would ever find any actual bug to justify the sweat it would take to maintain it. regards, tom lane
Re: Allow an alias to be attached directly to a JOIN ... USING
Hello Peter, I took another crack at this. Attached is a new patch that addresses the semantic comments from this and the other thread. It's all a bit tricky, comments welcome. It seems that this patch does not apply anymore after Tom's 5815696. -- Fabien.
Re: Recognizing superuser in pg_hba.conf
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > On Thu, Jan 2, 2020 at 15:50 Tom Lane wrote: > >> To cover the proposed functionality, you'd still need some way to > >> select not-superuser. So I don't think this fully answers the need > >> even if we wanted to do it. > > > Sorry- why do we need that..? The first match for a pg_hba line wins, so > > you can define all the access methods that superuser accounts are allowed > > to use first, then a “reject” line for superuser accounts, and then > > whatever else you want after that. > > Seems kind of awkward. Or more to the point: you can already do whatever > you want in pg_hba.conf, as long as you're willing to be verbose enough > (and, perhaps, willing to maintain group memberships to fit your needs). Sure it's awkward, but it's how people actually deal with these things today. I'm not against improving on that situation but I also don't hear tons of complaints about it either, so I do think we should be thoughtful when it comes to making changes here. > The discussion here, IMO, is about offering useful shorthands. In general, I'm alright with that idea, but I do want to make sure we're really being thoughtful when it comes to inventing new syntax that will only work in this one place and will have to be handled specially by any tools or anything that wants to generate or look at this. What are we going to have be displayed through pg_hba_file_rules() for this '!role' or whatever else, in the 'user_name' column? (Also, ugh, I find calling that column 'user_name' mildly offensive considering that function was added well after roles, and it's not like it really meant 'user name' even before then..). Yes, I'm sure we could just have it be the text '!role' and make everyone who cares have to parse out that field, in SQL, to figure out who it really applies to and basically just make everyone deal with it but I remain skeptical about if it's really a particularly good approach. > So a facility like "!role" seems potentially useful. Maybe it's not > really, but I don't think we should reject it just because there's > a verbose and non-obvious way to get the same result. I don't agree that it's "non-obvious" that if you have a config file where "first match wins" that things which don't match the first line are, by definition, NOT whatever that first line was and then fall through to the next, where you could use 'reject' if you want. In fact, I've always kinda figured that's what 'reject' was for, though I'll admit that it's been around for far longer than I've been involved in the project (sadly, I hadn't discovered PG yet back in 1998). Thanks, Stephen signature.asc Description: PGP signature
Re: sidewinder has one failure
On Fri, Jan 3, 2020 at 7:03 PM Amit Kapila wrote: > > On Fri, Jan 3, 2020 at 6:34 PM Mikael Kjellström > wrote: > > > > > > On 2020-01-03 13:01, Amit Kapila wrote: > > > > > 2020-01-02 19:51:05.687 CET [24138:3] FATAL: insufficient file > > > descriptors available to start server process > > > 2020-01-02 19:51:05.687 CET [24138:4] DETAIL: System allows 19, we > > > need at least 20. > > > 2020-01-02 19:51:05.687 CET [24138:5] LOG: database system is shut down > > > > > > Here, I think it is clear that the failure happens because we are > > > setting the value of max_files_per_process as 26 which is low for this > > > machine. It seems to me that the reason it is failing is that before > > > reaching set_max_safe_fds, it has already seven open files. Now, I > > > see on my CentOS system, the value of already_open files is 3, 6 and 6 > > > respectively for versions HEAD, 12 and 10. I debugged on HEAD and found that we are closing all the files (like postgresql.conf, postgresql.auto.conf, etc.) that got opened before set_max_safe_fds. I think on HEAD the 3 already opened files are basically stdin, stdout, stderr. It is still not clear why on some other versions it shows different number of already opened files. > > > We can easily see the > > > number of already opened files by changing the error level from DEBUG2 > > > to LOG for elog message in set_max_safe_fds. It is not very clear to > > > me how many files we can expect to be kept open during startup? Can > > > the number vary on different setups? > > > > Hm, where does it get the limit from? Is it something we set? > > > > Why is this machine different from everybody else when it comes to this > > limit? > > Mikael, is it possible for you to set log_min_messages to DEBUG2 on your machine and start the server. You must see a line like: "max_safe_fds = 984, usable_fds = 1000, already_open = 6". Is it possible to share that information? This is just to confirm if the already_open number is 7 on your machine. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Add support for automatically updating Unicode derived files
On Thu, Dec 26, 2019 at 12:39 PM Peter Eisentraut wrote: > > On 2019-12-19 23:48, John Naylor wrote: > > I would print out the full boilerplate like for other generated headers. > > Hmm, you are probably comparing with > src/common/unicode/generate-unicode_norm_table.pl, but other file > generating scripts around the tree print out a small header in the style > that I have. I'd rather adjust the output of > generate-unicode_norm_table.pl to match those. (It's also not quite > correct to make copyright claims about automatically generated output.) Hmm, the scripts I'm most familiar with have full headers. Your point about copyright makes sense, and using smaller file headers would aid readability of the scripts, but I also see how others may feel differently. v2 looks good to me, marked ready for committer. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Christoph Berg writes: > Shouldn't this print some "expected foo, got bar" diagnostics instead > of just dying? BTW, as far as that goes, we do: see for instance the tail end of https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=desmoxytes=2020-01-02%2020%3A04%3A03 ok 8 - offer multiple table choices ok 9 - finish completion of one of multiple table choices ok 10 - \r works not ok 11 - complete \DRD to \drds # Failed test 'complete \DRD to \drds' # at t/010_tab_completion.pl line 64. # Actual output was "\DRD" ok 12 - \r works Not sure why you are not seeing the "Actual output" bit in your log. I used a "note" command to print it, maybe that's not best practice? Also, while I'm asking for Perl advice: I can see in my editor that there's a control-G bell character in that string, but this is far from obvious on the web page. I'd kind of like to get the report to escapify control characters so that what comes out is more like # Actual output was "\DRD^G" or # Actual output was "\\DRD\007" or some such. Anybody know an easy way to do that in Perl? regards, tom lane
Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Christoph Berg writes: > Re: Tom Lane 2020-01-02 >> Add basic TAP tests for psql's tab-completion logic. > The \DRD test fails on Debian/unstable: Indeed. It appears that recent libedit breaks tab-completion for words involving a backslash, which is the fault of this upstream commit: http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/filecomplete.c.diff?r1=1.52=1.53 Basically what that's doing is applying de-backslashing to EVERY word that completion is attempted on, whether it might be a filename or not. So what psql_complete sees in this test case is just "DRD" which of course it does not recognize as a possible psql backslash command. I found out while investigating this that the libedit version shipping with buster (3.1-20181209) is differently broken for the same case: instead of inapproriate forced de-escaping of the input of the application-specific completion function, it applies inapproriate forced escaping to the output of said function, so that when we see "\DRD" and return "\drds", what comes out to the user is "\\drds". libedit apparently needs a regression test suite even worse than we do. I was kind of despairing of fixing this last night, but in the light of morning it occurs to me that there's a possible workaround for the de-escape bug: we could make psql_completion ignore the passed "text" string and look at the original input buffer, as get_previous_words() is already doing. I don't see any way to dodge buster's bug, though. regards, tom lane
Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes
Peter Eisentraut writes: > On 2019-11-03 23:40, Tom Lane wrote: >> * The patch now always quotes completed filenames, so quote_if_needed() >> is misnamed and overcomplicated for this use-case. I left the extra >> generality in place for possible future use. On the other hand, this >> is the*only* use-case, so you could also argue that we should just >> simplify the function's API. I have no strong opinion about that. > I haven't found an explanation in this thread why it does always quote > now. That seems a bit unusual. Is there a reason for this? Can we do > without it? The core problem we're trying to solve is stated in the thread title: if you do prompt# copy mytab from 'myfil then (assuming some completion is available) the current code actually *removes* the quote, which is completely wrong. Even if the user didn't type a leading quote, it's better to add one, because COPY won't work otherwise. It'd be possible, perhaps, to distinguish between this case and the cases in backslash commands, which are okay with omitted quotes (for some filenames). I'm not sure that that would be an improvement though. regards, tom lane
Re: sidewinder has one failure
On Fri, Jan 3, 2020 at 6:34 PM Mikael Kjellström wrote: > > > On 2020-01-03 13:01, Amit Kapila wrote: > > > 2020-01-02 19:51:05.687 CET [24138:3] FATAL: insufficient file > > descriptors available to start server process > > 2020-01-02 19:51:05.687 CET [24138:4] DETAIL: System allows 19, we > > need at least 20. > > 2020-01-02 19:51:05.687 CET [24138:5] LOG: database system is shut down > > > > Here, I think it is clear that the failure happens because we are > > setting the value of max_files_per_process as 26 which is low for this > > machine. It seems to me that the reason it is failing is that before > > reaching set_max_safe_fds, it has already seven open files. Now, I > > see on my CentOS system, the value of already_open files is 3, 6 and 6 > > respectively for versions HEAD, 12 and 10. We can easily see the > > number of already opened files by changing the error level from DEBUG2 > > to LOG for elog message in set_max_safe_fds. It is not very clear to > > me how many files we can expect to be kept open during startup? Can > > the number vary on different setups? > > Hm, where does it get the limit from? Is it something we set? > > Why is this machine different from everybody else when it comes to this > limit? > The problem we are seeing on this machine is that I think we have seven files opened before we reach function set_max_safe_fds during startup. Now, it is not clear to me why it is opening extra file(s) during start-up as compare to other machines. I think this kind of problem could occur if one has set shared_preload_libraries and via that, some file is getting opened which is not closed or there is some other configuration due to which this extra file is getting opened. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: WIP: System Versioned Temporal Table
On 03/01/2020 11:57, Surafel Temesgen wrote: > > > On Thu, Jan 2, 2020 at 12:12 AM Vik Fearing > mailto:vik.fear...@2ndquadrant.com>> wrote: > > This does not compile against current head (0ce38730ac). > > > gram.y: error: shift/reduce conflicts: 6 found, 0 expected > > > Rebased and conflict resolved i hope it build clean this time > It does but you haven't included your tests file so `make check` fails. It seems clear to me that you haven't tested it at all anyway. The temporal conditions do not return the correct results, and the syntax is wrong, too. Also, none of my previous comments have been addressed except for "system versioning" instead of "system_versioning". Why? -- Vik Fearing
Re: sidewinder has one failure
On 2020-01-03 13:01, Amit Kapila wrote: 2020-01-02 19:51:05.687 CET [24138:3] FATAL: insufficient file descriptors available to start server process 2020-01-02 19:51:05.687 CET [24138:4] DETAIL: System allows 19, we need at least 20. 2020-01-02 19:51:05.687 CET [24138:5] LOG: database system is shut down Here, I think it is clear that the failure happens because we are setting the value of max_files_per_process as 26 which is low for this machine. It seems to me that the reason it is failing is that before reaching set_max_safe_fds, it has already seven open files. Now, I see on my CentOS system, the value of already_open files is 3, 6 and 6 respectively for versions HEAD, 12 and 10. We can easily see the number of already opened files by changing the error level from DEBUG2 to LOG for elog message in set_max_safe_fds. It is not very clear to me how many files we can expect to be kept open during startup? Can the number vary on different setups? Hm, where does it get the limit from? Is it something we set? Why is this machine different from everybody else when it comes to this limit? ulimit -a says: $ ulimit -a time(cpu-seconds)unlimited file(blocks) unlimited coredump(blocks) unlimited data(kbytes) 262144 stack(kbytes)4096 lockedmem(kbytes)672036 memory(kbytes) 2016108 nofiles(descriptors) 1024 processes1024 threads 1024 vmemory(kbytes) unlimited sbsize(bytes)unlimited Is there any configuration setting I could do on the machine to increase this limit? /Mikael
Re: backup manifests
Thank you for review comments.
On Mon, Dec 30, 2019 at 11:53 PM Robert Haas wrote:
> On Tue, Dec 24, 2019 at 5:42 AM Suraj Kharage
> wrote:
> > To examine the first word of each line, I am using below check:
> > if (strncmp(line, "File", 4) == 0)
> > {
> > ..
> > }
> > else if (strncmp(line, "Manifest-Checksum", 17) == 0)
> > {
> > ..
> > }
> > else
> > error
> >
> > strncmp might be not right here, but we can not put '\0' in between the
> line (to find out first word)
> > before we recognize the line type.
> > All the lines expect line last one (where we have manifest checksum) are
> feed to the checksum machinary to calculate manifest checksum.
> > so update_checksum() should be called after recognizing the type, i.e:
> if it is a File type record. Do you see any issues with this?
>
> I see the problem, but I don't think your solution is right, because
> the first test would pass if the line said FiletMignon rather than
> just File, which we certainly don't want. You've got to write the test
> so that you're checking against the whole first word, not just some
> prefix of it. There are several possible ways to accomplish that, but
> this isn't one of them.
>
Yeah. Fixed in the attached patch.
>
> >> + pg_log_error("invalid record found in \"%s\"", manifest_path);
> >>
> >> Error message needs work.
>
> Looks better now, but you have a messages that say "invalid checksums
> type \"%s\" found in \"%s\"". This is wrong because checksums would
> need to be singular in this context (checksum). Also, I think it could
> be better phrased as "manifest file \"%s\" specifies unknown checksum
> algorithm \"%s\" at line %d".
>
Corrected.
>
> >> Your function names should be consistent with the surrounding style,
> >> and with each other, as far as possible. Three different conventions
> >> within the same patch and source file seems over the top.
>
> This appears to be fixed.
>
> >> Also keep in mind that you're not writing code in a vacuum. There's a
> >> whole file of code here, and around that, a whole project.
> >> scan_data_directory() is a good example of a function whose name is
> >> clearly too generic. It's not a general-purpose function for scanning
> >> the data directory; it's specifically a support function for verifying
> >> a backup. Yet, the name gives no hint of this.
>
> But this appears not to be fixed.
>
I have changed this function name to "VerifyDir" likewise, we have sendDir
and sendFile in basebackup.c
>
> >> if (strcmp(newpathsuffix, "/pg_wal") == 0 || strcmp(newpathsuffix,
> >> "/backup_manifest") == 0)
> >> continue;
> >
> > Thanks for the suggestion. Corrected as per the above inputs.
>
> You need a comment here, like "Ignore the possible presence of a
> backup_manifest file and/or a pg_wal directory in the backup being
> verified." and then maybe another sentence explaining why that's the
> right thing to do.
>
Corrected.
>
> + * The forth parameter to VerifyFile() will pass the relative
> path
> + * of file to match exactly with the filename present in
> manifest.
>
> I don't know what this comment is trying to tell me, which might be
> something you want to try to fix. However, I'm pretty sure it's
> supposed to say "fourth" not "forth".
>
I have changed the fourth parameter of VerifyFile(), so my comment over
there is no more valid.
>
> >> and the result would be that everything inside that long if-block is
> >> now at the top level of the function and indented one level less. And
> >> I think if you look at this function you'll see a way that you can
> >> save a *second* level of indentation for much of that code. Please
> >> check the rest of the patch for similar cases, too.
> >
> > Make sense. corrected.
>
> I don't agree. A large chunk of VerifyFile() is still subject to a
> quite unnecessary level of indentation.
>
Yeah, corrected.
>
> > I have added a check for EOF, but not sure whether that woule be right
> here.
> > Do we need to check the length of buffer as well?
>
> That's really, really not right. EOF is not a character that can
> appear in the buffer. It's chosen on purpose to be a value that never
> matches any actual character when both the character and the EOF value
> are regarded as values of type 'int'. That guarantee doesn't apply
> here though because you're dealing with values of type 'char'. So what
> this code is doing is searching for an impossible value using
> incorrect logic, which has very little to do with the actual need
> here, which is to avoid running off the end of the buffer. To see what
> the problem is, try creating a file with no terminating newline, like
> this:
>
> echo -n this file has no terminating newline >> some-file
>
> I doubt it will be very hard to make this patch crash horribly. Even
> if you can't, it seems pretty clear that the logic isn't right.
>
> I don't really know what the \0 tests in NextLine() and NextWord()
> think they're doing either. If there's a \0 in the buffer
Re: pgbench - allow to create partitioned tables
Hello Peter, The documentation and pgbench --help output that accompanied this patch claims that the argument to pgbench --partition-method is optional and defaults to "range", but that is not actually the case, as the implementation requires an argument. Could you please sort this out? AFAICS, if the user omits this argument, then the default is range as specified in docs. I tried by using something like 'pgbench.exe -i -s 1 --partitions=2 postgres' and then run 'pgbench -S postgres'. Ah, the way I interpreted this is that the argument to --partition-method itself is optional. Yep. Optionnal stuff would be in [], where () is used for choices. Would the attached have improved your understanding? It is somehow more consistent with other help lines. -- Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index a1e0663c8b..8d4f5f0866 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -639,7 +639,7 @@ usage(void) " --index-tablespace=TABLESPACE\n" " create indexes in the specified tablespace\n" " --partitions=NUM partition pgbench_accounts in NUM parts (default: 0)\n" - " --partition-method=(range|hash)\n" + " --partition-method=range|hash\n" " partition pgbench_accounts with this method (default: range)\n" " --tablespace=TABLESPACE create tables in the specified tablespace\n" " --unlogged-tablescreate tables as unlogged tables\n"
