Re: Direct I/O
On 2023-04-16 00:10, Tom Lane wrote: so curculio should be the only one that's at risk here. Maybe just upgrading it is the right answer. Just let me know if I should switch curculio to OpenBSD 7.3. I already have a new server setup so only need to switch the "animal" and "secret" and enable the cron job to get it running. /Mikael
Can we delete the vacuumdb.sgml notes about which version each option was added in?
I was just looking at the vacuumdb docs and noticed that I had neglected to follow the tradition of adding a note to mention which version we added the new option in when I committed the --buffer-usage-limit patch. There are 3 notes there that read "This option is only available for servers running PostgreSQL 9.6 and later.". Since 9.6 is a few years out of support, can we get rid of these 3? Or better yet, can we just delete them all? Is it really worth doing this in case someone is using a new vacuumdb on an older server? I just tried compiling the HTML with all the notes removed, I see from looking at a print preview that it's now ~1 full A4 page shorter than it was previously. 5 pages down to 4. Does anyone think we should keep these? David
Re: Can we delete the vacuumdb.sgml notes about which version each option was added in?
On Sun, Apr 16, 2023 at 10:14:35PM +1200, David Rowley wrote: > There are 3 notes there that read "This option is only available for > servers running PostgreSQL 9.6 and later.". Since 9.6 is a few years > out of support, can we get rid of these 3? +1 > Or better yet, can we just delete them all? Is it really worth doing > this in case someone is using a new vacuumdb on an older server? > > I just tried compiling the HTML with all the notes removed, I see from > looking at a print preview that it's now ~1 full A4 page shorter than > it was previously. 5 pages down to 4. > > Does anyone think we should keep these? I'm +0.5 for removing all of them. While they are still relevant and could potentially help users, these notes are taking up a rather big portion of the vacuumdb page, and it should print a nice error message if you try to use an option on and older server, anyway. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Can we delete the vacuumdb.sgml notes about which version each option was added in?
On Sun, Apr 16, 2023 at 10:14:35PM +1200, David Rowley wrote: > I was just looking at the vacuumdb docs and noticed that I had > neglected to follow the tradition of adding a note to mention which > version we added the new option in when I committed the > --buffer-usage-limit patch. > > There are 3 notes there that read "This option is only available for > servers running PostgreSQL 9.6 and later.". Since 9.6 is a few years > out of support, can we get rid of these 3? > > Or better yet, can we just delete them all? Is it really worth doing > this in case someone is using a new vacuumdb on an older server? > > I just tried compiling the HTML with all the notes removed, I see from > looking at a print preview that it's now ~1 full A4 page shorter than > it was previously. 5 pages down to 4. > > Does anyone think we should keep these? I don't know if I'd support removing the notes, but I agree that they don't need to take up anywhere near as much space as they do (especially since the note is now repeated 10 times). https://www.postgresql.org/docs/devel/app-vacuumdb.html I suggest to remove the markup and preserve the annotation about version compatibility. It's normal, technical writing to repeat the same language like that. Another, related improvement I suggested would be to group the client-side options separately from the server-side options. https://www.postgresql.org/message-id/zbydtrd1kygg+...@telsasoft.com -- Justin
Re: Direct I/O
=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= writes: > On 2023-04-16 00:10, Tom Lane wrote: >> so curculio should be the only one that's at risk here. >> Maybe just upgrading it is the right answer. > Just let me know if I should switch curculio to OpenBSD 7.3. Yes please. > I already have a new server setup so only need to switch the "animal" > and "secret" and enable the cron job to get it running. Actually, as long as it's still OpenBSD I think you can keep using the same animal name ... Andrew, what's the policy on that? regards, tom lane
Re: idea: multiple arguments to_regclass function
Pavel Stehule writes: > I missing some variants of to_regclass > to_regclass(schemaname, objectname) > to_regclass(catalogname, schemaname, objectname) Can do that already: to_regclass(quote_ident(schemaname) || '.' || quote_ident(objectname)) I'm not eager to build nine more to_reg* functions to do the equivalent of that, and even less eager to build eighteen. regards, tom lane
Re: Can we delete the vacuumdb.sgml notes about which version each option was added in?
Justin Pryzby writes: > On Sun, Apr 16, 2023 at 10:14:35PM +1200, David Rowley wrote: >> Does anyone think we should keep these? > I don't know if I'd support removing the notes, but I agree that they > don't need to take up anywhere near as much space as they do (especially > since the note is now repeated 10 times). I agree with removing the notes. It has always been our policy that you should read the version of the manual that applies to the version you're running. I can see leaving a compatibility note around for a long time when it's warning you that some behavior changed compared to what the same syntax used to do. But if a switch simply isn't there in some older version, that's not terribly dangerous or hard to figure out. > I suggest to remove the markup and preserve the annotation about > version compatibility. It's normal, technical writing to repeat the > same language like that. Another way could be to move them all into a "Compatibility" section. But +1 for just dropping them. regards, tom lane
Re: Direct I/O
On 2023-04-16 16:18, Tom Lane wrote: =?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= writes: On 2023-04-16 00:10, Tom Lane wrote: so curculio should be the only one that's at risk here. Maybe just upgrading it is the right answer. Just let me know if I should switch curculio to OpenBSD 7.3. Yes please. Ok. I already have a new server setup so only need to switch the "animal" and "secret" and enable the cron job to get it running. Actually, as long as it's still OpenBSD I think you can keep using the same animal name ... Andrew, what's the policy on that? That is what I meant with above. I just use the same animal name and secret and then run "update_personality.pl". That should be enough I think? /Mikael
Re: Direct I/O
On 2023-04-16 Su 10:18, Tom Lane wrote: =?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= writes: On 2023-04-16 00:10, Tom Lane wrote: so curculio should be the only one that's at risk here. Maybe just upgrading it is the right answer. Just let me know if I should switch curculio to OpenBSD 7.3. Yes please. I already have a new server setup so only need to switch the "animal" and "secret" and enable the cron job to get it running. Actually, as long as it's still OpenBSD I think you can keep using the same animal name ... Andrew, what's the policy on that? update_personality.pl lets you update the OS version / compiler version / owner-name / owner-email I am in fact about to perform this exact operation for prion. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: idea: multiple arguments to_regclass function
ne 16. 4. 2023 v 16:23 odesílatel Tom Lane napsal:
> Pavel Stehule writes:
> > I missing some variants of to_regclass
>
> > to_regclass(schemaname, objectname)
> > to_regclass(catalogname, schemaname, objectname)
>
> Can do that already:
>
> to_regclass(quote_ident(schemaname) || '.' || quote_ident(objectname))
>
> I'm not eager to build nine more to_reg* functions to do the equivalent
> of that, and even less eager to build eighteen.
>
Yes, I can. But there is overhead with escaping and string concatenation.
And it is a little bit sad, so immediately the parser has to do an inverse
process.
Maybe we can introduce only three functions
anyelement get_object(catalogname name, schemaname name, objectname name,
returntype anyelement)
anyelement get_object(schemaname name, objectname name, returntype
anyelement)
anyelement get_object(objectname name, returntype anyelement)
so usage can be like
DECLATE _tab regclass;
BEGIN
_tab := get_object('public', 'mytab', _tab);
..
?
Regards
Pavel
> regards, tom lane
>
Re: Direct I/O
Andrew Dunstan writes: > On 2023-04-16 Su 10:18, Tom Lane wrote: >> Actually, as long as it's still OpenBSD I think you can keep using >> the same animal name ... Andrew, what's the policy on that? > update_personality.pl lets you update the OS version / compiler version > / owner-name / owner-email Oh wait ... this involves a switch from gcc in OpenBSD 5.9 to clang in OpenBSD 7.3, doesn't it? That isn't something update_personality will handle; you need a new animal if the compiler product is changing. regards, tom lane
Re: v16dev: invalid memory alloc request size 8488348128
Justin Pryzby writes: > On Sat, Apr 15, 2023 at 11:33:58AM +1200, David Rowley wrote: >> Any chance you could try and come up with a minimal reproducer? > Try this Thanks. I see the problem: finalize_aggregate is no longer forcing a R/W expanded datum returned by the finalfn into R/O form. If we re-use the aggregate result in multiple places, as this query does, then the first use can clobber the value for later uses. (The commit message specifically mentions this concern, so I wonder how we failed to actually do it :-() A minimal fix would be to force to R/O before returning from finalize_aggregate, but I wonder if we should do it later. By the by, I couldn't help noticing that ExecAggTransReparent completely fails to do what its name promises it should do, ie reparent a R/W datum into the proper context instead of physically copying it. That looks suspiciously like something that got broken during some other refactoring somewhere along the line. That'd be a performance bug not a correctness bug, but it should be looked into. regards, tom lane
Re: Direct I/O
> On Apr 16, 2023, at 12:16 PM, Tom Lane wrote: > > Andrew Dunstan writes: >>> On 2023-04-16 Su 10:18, Tom Lane wrote: >>> Actually, as long as it's still OpenBSD I think you can keep using >>> the same animal name ... Andrew, what's the policy on that? > >> update_personality.pl lets you update the OS version / compiler version >> / owner-name / owner-email > > Oh wait ... this involves a switch from gcc in OpenBSD 5.9 to clang > in OpenBSD 7.3, doesn't it? That isn't something update_personality > will handle; you need a new animal if the compiler product is changing. > > Correct. Cheers Andrew
Re: Direct I/O
On 2023-04-16 19:59, Andrew Dunstan wrote: On Apr 16, 2023, at 12:16 PM, Tom Lane wrote: Andrew Dunstan writes: On 2023-04-16 Su 10:18, Tom Lane wrote: Actually, as long as it's still OpenBSD I think you can keep using the same animal name ... Andrew, what's the policy on that? update_personality.pl lets you update the OS version / compiler version / owner-name / owner-email Oh wait ... this involves a switch from gcc in OpenBSD 5.9 to clang in OpenBSD 7.3, doesn't it? That isn't something update_personality will handle; you need a new animal if the compiler product is changing. Correct. OK. I registered a new animal for this then. So if someone could look at that and give be an animal name + secret I can set this up. /Mikael
[PATCH] Add support for postgres:// URIs to PGDATABASE environment variable
The PGDATABASE is documented as behaving the same as the dbname connection
parameter but they differ in the support for postgres:// URIs: the
PGDATABASE will never be expanded even thought expand_dbname is set:
$ psql postgres://localhost/test -c 'select 1' >/dev/null # Works
$ PGDATABASE=postgres://localhost/test psql -c 'select 1' >/dev/null #
Doesn't work
psql: error: connection to server on socket "/tmp/.s.PGSQL.5432"
failed: FATAL: database "postgres://localhost/test" does not exist
In the second command the postgres://localhost/test string has not been
interpreted and the connection fails.
In order to make PGDATABASE and dbname behave the same this patch adds
URIs support to the environment variable. This makes it convenient for
users to pass a single connection string when environment variables are
used.
When both PGDATABASE and dbname are a connection string, the values of
dbname will override the ones of PGDATABASE, e.g.
$ PGDATABASE=postgres://localhost/test?sslmode=require psql
postgres://host/db
is equivalent to
$ psql postgres://host/db?sslmode=require
I did not write tests for this patch as I am not sure where to put them
since libpq_uri_regress uses PQconninfoParse() that does not read the
environment variables.
---
src/interfaces/libpq/fe-connect.c | 52 +++
1 file changed, 46 insertions(+), 6 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index fcd3d0d9a3..64d6685ad8 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -412,7 +412,7 @@ static PQconninfoOption *conninfo_array_parse(const char
*const *keywords,
const char *const *values, PQExpBuffer errorMessage,
bool use_defaults, int expand_dbname);
static bool conninfo_add_defaults(PQconninfoOption *options,
- PQExpBuffer
errorMessage);
+ PQExpBuffer
errorMessage, int expand_dbname);
static PQconninfoOption *conninfo_uri_parse(const char *uri,
PQExpBuffer errorMessage, bool use_defaults);
static bool conninfo_uri_parse_options(PQconninfoOption *options,
@@ -1791,7 +1791,7 @@ PQconndefaults(void)
if (connOptions != NULL)
{
/* pass NULL errorBuf to ignore errors */
- if (!conninfo_add_defaults(connOptions, NULL))
+ if (!conninfo_add_defaults(connOptions, NULL, false))
{
PQconninfoFree(connOptions);
connOptions = NULL;
@@ -6084,7 +6084,7 @@ conninfo_parse(const char *conninfo, PQExpBuffer
errorMessage,
*/
if (use_defaults)
{
- if (!conninfo_add_defaults(options, errorMessage))
+ if (!conninfo_add_defaults(options, errorMessage, false))
{
PQconninfoFree(options);
return NULL;
@@ -6250,7 +6250,7 @@ conninfo_array_parse(const char *const *keywords, const
char *const *values,
*/
if (use_defaults)
{
- if (!conninfo_add_defaults(options, errorMessage))
+ if (!conninfo_add_defaults(options, errorMessage,
expand_dbname))
{
PQconninfoFree(options);
return NULL;
@@ -6272,7 +6272,7 @@ conninfo_array_parse(const char *const *keywords, const
char *const *values,
* NULL.
*/
static bool
-conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
+conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage, int
expand_dbname)
{
PQconninfoOption *option;
PQconninfoOption *sslmode_default = NULL,
@@ -6296,6 +6296,46 @@ conninfo_add_defaults(PQconninfoOption *options,
PQExpBuffer errorMessage)
if (strcmp(option->keyword, "sslrootcert") == 0)
sslrootcert = option; /* save for later */
+ if (expand_dbname && strcmp(option->keyword, "dbname") == 0)
+ {
+ if ((tmp = getenv(option->envvar)) != NULL &&
recognized_connection_string(tmp))
+ {
+ PQconninfoOption *str_option,
+
*dbname_options = parse_connection_string(tmp, errorMessage, false);
+
+ if (dbname_options == NULL)
+ return false;
+
+ for (str_option = dbname_options;
str_option->keyword != NULL; str_option++)
+
Re: check_strxfrm_bug()
On Sun, Dec 18, 2022 at 10:27 AM Thomas Munro wrote: > With my garbage collector hat on, that made me wonder if there was > some more potential cleanup here: could we require locale_t yet? The > last straggler systems on our target OS list to add the POSIX locale_t > stuff were Solaris 11.4 (2018) and OpenBSD 6.2 (2018). Apparently > it's still too soon: we have two EOL'd OSes in the farm that are older > than that. But here's an interesting fact about wrasse, assuming its > host is gcc211: it looks like it can't even apply further OS updates > because the hardware[1] is so old that Solaris doesn't support it > anymore[2]. For the record, now the OpenBSD machines have been upgraded, so now "wrasse" is the last relevant computer on earth with no POSIX locale_t. Unfortunately there is no reason to think it's going to go away soon, so I'm just noting this fact here as a reminder for when it eventually does...
Re: Can we delete the vacuumdb.sgml notes about which version each option was added in?
On Mon, 17 Apr 2023 at 02:29, Tom Lane wrote: > But +1 for just dropping them. Thanks. I just pushed the patch to drop them all. David
Re: Direct I/O
On Sun, Apr 16, 2023 at 04:51:04PM +0200, Mikael Kjellström wrote: > That is what I meant with above. > > I just use the same animal name and secret and then run > "update_personality.pl". > > That should be enough I think? Yes, that should be enough as far as I recall. This has been mentioned a couple of weeks ago here: https://www.postgresql.org/message-id/ca+hukgk0jj+g+bxluzqpbsxpveg7lvt1v8lbxfkzbrvtfts...@mail.gmail.com I have also used setnotes.pl to reflect my animals' CFLAGS on the website. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Add support for postgres:// URIs to PGDATABASE environment variable
=?UTF-8?q?R=C3=A9mi=20Lapeyre?= writes: > The PGDATABASE is documented as behaving the same as the dbname connection > parameter but they differ in the support for postgres:// URIs: the > PGDATABASE will never be expanded even thought expand_dbname is set: I think you have misunderstood the documentation. What you are proposing is equivalent to saying that this should work: $ psql -d "dbname=postgres://localhost/test" psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: database "postgres://localhost/test" does not exist That doesn't work, never has, and I think it would be a serious compatibility break and possibly a security hazard if it did. The argument of "dbname=" should not be subject to another round of interpretation, and neither should the content of the PGDATABASE environment variable. You can do this: $ psql -d "postgres://localhost/test" but that's not the same thing as reinterpreting the dbname field of what we have already determined to be a connection string. Perhaps there is a case for inventing a new environment variable that can do what you're suggesting. But you would have to make a case that it's worth doing, and also define how it interacts with all the other PGxxx environment variables. (The lack of clarity about how that should work is an important part of why I don't like the idea of letting dbname/PGDATABASE supply anything but the database name.) regards, tom lane
Re: longfin missing gssapi_ext.h
Stephen Frost writes: > Pushed, thanks again to everyone. > I'll monitor the buildfarm and assuming there isn't anything unexpected > then I'll mark the open item as resolved now. The Debian 7 (Wheezy) members of the buildfarm (lapwing, skate, snapper) are all getting past the gssapi_ext.h check you added and then failing like this: ccache gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -Werror -I../../../src/include -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o be-gssapi-common.o be-gssapi-common.c be-gssapi-common.c: In function 'pg_store_delegated_credential': be-gssapi-common.c:110:2: error: unknown type name 'gss_key_value_element_desc' be-gssapi-common.c:111:2: error: unknown type name 'gss_key_value_set_desc' be-gssapi-common.c:113:4: error: request for member 'key' in something not a structure or union be-gssapi-common.c:114:4: error: request for member 'value' in something not a structure or union be-gssapi-common.c:115:7: error: request for member 'count' in something not a structure or union be-gssapi-common.c:116:7: error: request for member 'elements' in something not a structure or union be-gssapi-common.c:119:2: error: implicit declaration of function 'gss_store_cred_into' [-Werror=implicit-function-declaration] Debian 7 has been EOL five years or so, so I don't mind saying "get a newer OS or disable gssapi". However, is it worth adding another configure check to fail a little faster with whatever Kerberos version this is? Checking that gss_store_cred_into() exists seems like the most obvious one of these things to test for. regards, tom lane
Re: Should vacuum process config file reload more often
On Wed, Apr 12, 2023 at 12:05 AM Masahiko Sawada wrote: > > On Fri, Apr 7, 2023 at 10:23 PM Daniel Gustafsson wrote: > > > > > On 7 Apr 2023, at 15:07, Melanie Plageman > > > wrote: > > > On Fri, Apr 7, 2023 at 2:53 AM Masahiko Sawada > > > wrote: > > > > > > Definitely seems worth fixing as it kind of defeats the purpose of the > > > original commit. I wish I had noticed before! > > > > > > Your fix has: > > >!(avopts && (avopts->vacuum_cost_limit >= 0 || > > >avopts->vacuum_cost_delay >= 0)); > > > > > > And though delay is required to be >= 0 > > >avopts->vacuum_cost_delay >= 0 > > > > > > Limit does not. It can just be > 0. > > > > > > postgres=# create table foo (a int) with (autovacuum_vacuum_cost_limit = > > > 0); > > > ERROR: value 0 out of bounds for option "autovacuum_vacuum_cost_limit" > > > DETAIL: Valid values are between "1" and "1". > > > > > > Though >= is also fine, the rest of the code in all versions always > > > checks if limit > 0 and delay >= 0 since 0 is a valid value for delay > > > and not for limit. Probably best we keep it consistent (though the whole > > > thing is quite confusing). > > > > +1 > > +1. I misunderstood the initial value of autovacuum_vacuum_cost_limit > reloption. I've attached an updated patch for fixing at_dobalance condition. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com 0001-Fix-the-condition-of-joining-autovacuum-workers-to-b.patch Description: Binary data
Re: Allowing parallel-safe initplans
On Thu, Apr 13, 2023 at 10:00 PM Tom Lane wrote: > Richard Guo writes: > > * For the diff in standard_planner, I was wondering why not move the > > initPlans up to the Gather node, just as we did before. So I tried that > > way but did not notice the breakage of regression tests as stated in the > > comments. Would you please confirm that? > > Try it with debug_parallel_query = regress. Ah, I see. With DEBUG_PARALLEL_REGRESS the initPlans that move to the Gather would become invisible along with the Gather node. As I tried this, I found that the breakage caused by moving the initPlans to the Gather node might be more than just being cosmetic. Sometimes it may cause wrong results. As an example, consider create table a (i int, j int); insert into a values (1, 1); create index on a(i, j); set enable_seqscan to off; set debug_parallel_query to on; # select min(i) from a; min - 0 (1 row) As we can see, the result is not correct. And the plan looks like # explain (verbose, costs off) select min(i) from a; QUERY PLAN --- Gather Output: ($0) Workers Planned: 1 Single Copy: true InitPlan 1 (returns $0) -> Limit Output: a.i -> Index Only Scan using a_i_j_idx on public.a Output: a.i Index Cond: (a.i IS NOT NULL) -> Result Output: $0 (12 rows) The initPlan has been moved from the Result node to the Gather node. As a result, when doing tuple projection for the Result node, we'd get a ParamExecData entry with NULL execPlan. So the initPlan does not get chance to be executed. And we'd get the output as the default value from the ParamExecData entry, which is zero as shown. So now I begin to wonder if this wrong result issue is possible to exist in other places where we move initPlans. But I haven't tried hard to verify that. Thanks Richard
Re: Initial Schema Sync for Logical Replication
On Fri, Apr 7, 2023 at 6:37 PM Amit Kapila wrote:
>
> On Thu, Apr 6, 2023 at 6:57 PM Masahiko Sawada wrote:
> >
> > On Thu, Mar 30, 2023 at 10:11 PM Masahiko Sawada
> > wrote:
> > >
> > > On Thu, Mar 30, 2023 at 12:18 AM Masahiko Sawada
> > > wrote:
> > > >
> > > >
> > > > How can we postpone creating the pg_subscription_rel entries until the
> > > > tablesync worker starts and does the schema sync? I think that since
> > > > pg_subscription_rel entry needs the table OID, we need either to do
> > > > the schema sync before creating the entry (i.e, during CREATE
> > > > SUBSCRIPTION) or to postpone creating entries as Amit proposed[1]. The
> > > > apply worker needs the information of tables to sync in order to
> > > > launch the tablesync workers, but it needs to create the table schema
> > > > to get that information.
> > >
> > > For the above reason, I think that step 6 of the initial proposal won't
> > > work.
> > >
> > > If we can have the tablesync worker create an entry of
> > > pg_subscription_rel after creating the table, it may give us the
> > > flexibility to perform the initial sync. One idea is that we add a
> > > relname field to pg_subscription_rel so that we can create entries
> > > with relname instead of OID if the table is not created yet. Once the
> > > table is created, we clear the relname field and set the OID of the
> > > table instead. It's not an ideal solution but we might make it simpler
> > > later.
> >
> > While writing a PoC patch, I found some difficulties in this idea.
> > First, I tried to add schemaname+relname to pg_subscription_rel but I
> > could not define the primary key of pg_subscription_rel. The primary
> > key on (srsubid, srrelid) doesn't work since srrelid could be NULL.
> > Similarly, the primary key on (srsubid, srrelid, schemaname, relname)
> > also doesn't work.
> >
>
> Can we think of having a separate catalog table say
> pg_subscription_remote_rel for this? You can have srsubid,
> remote_schema_name, remote_rel_name, etc. We may need some other state
> to be maintained during the initial schema sync where this table can
> be used. Basically, this can be used to maintain the state till the
> initial schema sync is complete because we can create a relation entry
> in pg_subscritption_rel only after the initial schema sync is
> complete.
It might not be ideal but I guess it works. But I think we need to
modify the name of replication slot for initial sync as it currently
includes OID of the table:
void
ReplicationSlotNameForTablesync(Oid suboid, Oid relid,
char *syncslotname, Size szslot)
{
snprintf(syncslotname, szslot, "pg_%u_sync_%u_" UINT64_FORMAT, suboid,
relid, GetSystemIdentifier());
}
If we use both schema name and table name, it's possible that slot
names are duplicated if schema and/or table names are long. Another
idea is to use the hash value of schema+table names, but it cannot
completely eliminate that possibility, and probably would make
investigation and debugging hard in case of any failure. Probably we
can use the OID of each entry in pg_subscription_remote_rel instead,
but I'm not sure it's a good idea, mainly because we will end up using
twice as many OIDs as before.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Re: postgres_fdw: wrong results with self join + enable_nestloop off
Thanks Etsuro for your response! One small typo correction in my answer to "What is the technical issue?" "it is *not* considered a pseudo constant" --> "it is considered a pseudo constant" Regards, Nishant. On Fri, Apr 14, 2023 at 6:21 PM Etsuro Fujita wrote: > Hi Nishant, > > On Fri, Apr 14, 2023 at 8:39 PM Nishant Sharma > wrote: > > I debugged this issue and was able to find a fix for the same. Kindly > please refer to the attached fix. With the fix I am able to resolve the > issue. > > Thanks for the report and patch! > > > What is the technical issue? > > The problem here is the use of extract_actual_clauses. Because of which > the plan creation misses adding the second condition of AND i.e "now() < > '23-Feb-2020'::timestamp" in the plan. Because it is not considered a > pseudo constant and extract_actual_clause is passed with false as the > second parameter and it gets skipped from the list. As a result that > condition is never taken into consideration as either one-time filter > (before or after) or part of SQL remote execution > > > > Why do I think the fix is correct? > > The fix is simple, where we have created a new function similar to > extract_actual_clause which just extracts all the conditions from the list > with no checks and returns the list to the caller. As a result all > conditions would be taken into consideration in the query plan. > > I think that the root cause for this issue would be in the > create_scan_plan handling of pseudoconstant quals when creating a > foreign-join (or custom-join) plan. Anyway, I will look at your patch > closely, first. > > Best regards, > Etsuro Fujita >
Re: allow_in_place_tablespaces vs. pg_basebackup
On Fri, Apr 14, 2023 at 04:11:47PM -0400, Robert Haas wrote: > Do people think it's OK to do that now, or should I wait until we've > branched? I personally think this is bug-fix-ish enough that now is > OK, but I'll certainly forebear if others disagree. FWIW, doing that now rather than the beginning of July is OK for me for this stuff. -- Michael signature.asc Description: PGP signature
Re: Direct I/O
On 2023-04-16 20:05, Mikael Kjellström wrote: Oh wait ... this involves a switch from gcc in OpenBSD 5.9 to clang in OpenBSD 7.3, doesn't it? That isn't something update_personality will handle; you need a new animal if the compiler product is changing. Correct. OK. I registered a new animal for this then. So if someone could look at that and give be an animal name + secret I can set this up. I have setup a new animal "schnauzer" (thanks andrew!). That should report in a little while. /Mikael
Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
On Fri, Apr 14, 2023 at 11:28 PM Fujii Masao wrote: > On 2023/04/14 18:59, Etsuro Fujita wrote: > >> The primary message basically should avoid reference to implementation > >> details such as specific structure names like PGcancel, shouldn't it, as > >> per the error message style guide? > > > > I do not think that PGcancel is that specific, as it is described in > > the user-facing documentation [1]. (In addition, the error message I > > proposed was created by copying the existing error message "could not > > create OpenSSL BIO structure" in contrib/sslinfo.c.) > > I think that mentioning PGcancel in the error message could be confusing for > average users who are just running a query on a foreign table and encounter > the error message after pressing Ctrl-C. They may not understand why the > PGcancel struct is referenced in the error message while accessing foreign > tables. It could be viewed as an internal detail that is not necessary for > the user to know. Ok, understood. I do not think it is wrong to use "could not send cancel request” for PQgetCancel as well, but I feel that that is not perfect for PQgetCancel, because that function never sends a cancel request; that function just initializes the request. So how about "could not initialize cancel request”, instead? > >> Although the primary message is the same, the supplemental message > >> provides additional context that can help distinguish which function is > >> reporting the message. > > > > If the user is familiar with the PQgetCancel/PQcancel internals, this > > is true, but if not, I do not think this is always true. Consider > > this error message, for example: > > > > 2023-04-14 17:48:55.862 JST [24344] WARNING: could not send cancel > > request: invalid integer value "999" for connection option > > "keepalives" > > > > It would be hard for users without the knowledge about those internals > > to distinguish that from this message. For average users, I think it > > would be good to use a more distinguishable error message. > > In this case, I believe that they should be able to understand that an > invalid integer value "999" was specified in the "keepalives" > connection option, which caused the warning message. Then, they would need to > check the setting of the "keepalives" option and correct it if necessary. Maybe my explanation was not clear. Let me explain. Assume that a user want to identify the place where the above error was thrown. Using grep with ”could not send cancel request”, the user can find the two places emitting the message in pgfdw_cancel_query_begin: one for PQgetCancel and one for PQcancel. If the user are familiar with the PQgetCancel/PQcancel internals, the user can determine, from the supplemental message, that the error was thrown by the former. But if not, the user cannot do so. To support the unfamiliar user as well, I think it would be a good idea to use a more appropriate message for PQgetCancel that is different from "could not send cancel request”. (I agree that most users would not care about the places where errors were thrown, but I think some users would, and actually, I do when investigating unfamiliar errors.) Best regards, Etsuro Fujita
