Re: pg_get_publication_tables() output duplicate relid

[Amit Kapila]

On Fri, Nov 19, 2021 at 10:58 AM Amit Kapila  wrote:
>
> On Fri, Nov 19, 2021 at 7:19 AM Amit Langote  wrote:
> >
> > The problematic case is attaching the partition *after* the subscriber
> > has already marked the root parent as synced and/or ready for
> > replication.  Refreshing the subscription doesn't help it discover the
> > newly attached partition, because a publish_via_partition_root only
> > ever tells about the root parent, which would be already synced, so
> > the subscriber would think there's nothing to copy.
> >
>
> Okay, I see this could be a problem but I haven't tried to reproduce it.
>
> > > Anyway, if this is a problem
> > > we need to figure the solution for this separately.
> >
> > Sure, we might need to do that after all.  Though it might be a good
> > idea to be sure that we won't need to reconsider the fix we push for
> > the issue(s) being discussed here and elsewhere, because I suspect
> > that the solution to the problem I mentioned is likely to involve
> > tweaking pg_publication_tables view output.
> >

I have thought about this problem and I see two possibilities for a
solution (a) We could provide a new option say 'truncate' (something
on lines proposed here [1]) which would truncate the table(s) and
change its status to 'i' in the pg_subscription_rel, this would allow
the newly added partition to be synced after refresh. This could lead
to a large copy in such a case. (b) We could somehow get and store all
the partition info from the publisher-side on the subscriber-side
while initial sync (say in new system table
pg_subscription_rel_members). Now, after the refresh, if this list
changes, we can allow to just get the data of that particular
partition but I guess it would mean that we need to store oids of the
publisher which might or might not be safe considering oids can
wraparound before the refresh.

Do you have any other ideas?

One more thing you mentioned is that the initial sync won't work after
refresh but later changes will be replicated but I noticed that later
changes also don't get streamed till we restart the subscriber server.
I am not sure but we might not be invalidating apply workers cache due
to which it didn't notice the same.

[1] - 
https://www.postgresql.org/message-id/CF3B6672-2A43-4204-A60A-68F359218A9B%40endpoint.com

-- 
With Regards,
Amit Kapila.

Re: Pasword expiration warning

[Andrew Dunstan]

On 11/19/21 19:17, Bossart, Nathan wrote:
> On 11/19/21, 7:56 AM, "Tom Lane"  wrote:
>> That leads me to wonder about server-side solutions.  It's easy
>> enough for the server to see that it's used a password with an
>> expiration N days away, but how could that be reported to the
>> client?  The only idea that comes to mind that doesn't seem like
>> a protocol break is to issue a NOTICE message, which doesn't
>> seem like it squares with your desire to only do this interactively.
>> (Although I'm not sure I believe that's a great idea.  If your
>> application breaks at 2AM because its password expired, you
>> won't be any happier than if your interactive sessions start to
>> fail.  Maybe a message that would leave a trail in the server log
>> would be best after all.)
> I bet it's possible to use the ClientAuthentication_hook for this.  In
> any case, I agree that it probably belongs server-side so that other
> clients can benefit from this.
>

+1 for a server side solution. The people most likely to benefit from
this are the people least likely to be using psql IMNSHO.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Should rename "startup process" to something else?

[Andrew Dunstan]

On 11/18/21 15:22, Tom Lane wrote:
> Alvaro Herrera  writes:
>> If we change the name, and I support the idea that we do, I think a
>> good name would be "wal replay".  I think "recovery" is not great
>> precisely because in a standby there is likely no crash that we're
>> recovering from.
> Fair point.
>
>> The word "replay" is at odds with the other names,
>> which stand for the device that carries out the task at hand
>> (checkpointer, bgwriter, wal sender/receiver); but the word "replayer"
>> seems to be extremely uncommon and IMO looks strange.  If you see a
>> process that claims to be "wal replay", you know perfectly well what it
>> is.
> I'm less concerned about the "er" than about the fact that the name is
> two words.  People will immediately shorten it to just "replay", eg
> as a part of names in the code, and I feel that that's confusing in
> its own way.  Maybe we could run the words together, on the precedent
> of "walreceiver", but I never much liked that name either.
>
>   



Maybe something along those lines but using a dash/hyphen would work:
e.g. wal-replayer


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Test::More version

[Andrew Dunstan]

On 11/15/21 12:43, Tom Lane wrote:
>
> So, discounting prairiedog's intentionally trailing-edge installation,
> the oldest stuff in the buildfarm is 0.98, of which there are five
> instances belonging to four different owners.
>
> Based on this, I'm inclined to think we should select 0.98 as the
> minimum version.  Anything later would inconvenience some people.
> OTOH, it doesn't look like there's any value in promising compatibility
> with 0.96 instead, especially since I see a couple of subplan-related
> bug fixes in 0.97 and 0.98 in Test::Simple's changelog.
>
>   



Yeah, so I think at this stage we're just waiting for you to update
prairiedog and we can make this change.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

rename SnapBuild* macros in slot.c

[Bharath Rupireddy]

Hi,

It seems like the same macro names for
SnapBuildOnDiskNotChecksummedSize and SnapBuildOnDiskChecksummedSize
are being used in slot.c and snapbuild.c. I think, in slot.c, we can
rename them to ReplicationSlotOnDiskNotChecksummedSize and
ReplicationSlotOnDiskChecksummedSize
similar to the other macros ReplicationSlotOnDiskConstantSize and
ReplicationSlotOnDiskV2Size.

Here's a tiny patch for the above change.

Regards,
Bharath Rupireddy.


v1-0001-rename-SnapBuild-macros-in-slot.c.patch
Description: Binary data

Re: update with no changes

[Andrew Dunstan]

On 11/19/21 12:57, Marcos Pegoraro wrote:
>
> I get the idea of letting the server centralize logic like this -
> but frankly if the application is choosing to send all that data
> across the wire just to have the server throw it away the
> application is wasting network I/O.  If it does manage its
> resources carefully then the server will never even see an update
> and its behavior here becomes moot.
>
> I understand your point, it´s responsability of application to do what
> it has to do. But lots of times (maybe 98% of them) is not same people
> doing server side and application side. So, Postgres guys will have to
> review all code being done on apps ?
>
> And ok, thanks for explaining me.


suppress_redundant_updates_trigger was created precisely because it's
not always easy to create application code in such a way that it
generates no redundant updates. However, there is a cost to using it,
and the break even point can be surprisingly high. It should therefore
be used with caution, and after appropriate benchmarks.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: update with no changes

[Marcos Pegoraro]

>
> suppress_redundant_updates_trigger was created precisely because it's
> not always easy to create application code in such a way that it
> generates no redundant updates. However, there is a cost to using it,
> and the break even point can be surprisingly high. It should therefore
> be used with caution, and after appropriate benchmarks.
>
> well, there is a cost of not using it too. If lots of things needs to be
done when a record is stored, and if it doesn´t needed to be stored, all
these things will not be done.  So, what are pros of changing a record
which did not changed any value and what are cons of it ? So, I understood
the way it works and yes, my point of view is that this trigger is really
needed, for me, obviously.

Re: update with no changes

[Andrew Dunstan]

On 11/20/21 10:03, Marcos Pegoraro wrote:
>
> suppress_redundant_updates_trigger was created precisely because it's
> not always easy to create application code in such a way that it
> generates no redundant updates. However, there is a cost to using it,
> and the break even point can be surprisingly high. It should therefore
> be used with caution, and after appropriate benchmarks.
>
> well, there is a cost of not using it too. If lots of things needs to
> be done when a record is stored, and if it doesn´t needed to be
> stored, all these things will not be done.  So, what are pros of
> changing a record which did not changed any value and what are cons of
> it ? So, I understood the way it works and yes, my point of view is
> that this trigger is really needed, for me, obviously.



If you need it then use it. It's been built into postgres since release
8.4. Just be aware that if you use it there is a cost incurred for every
record updated whether or not the record is redundant. If only 12% of
your updates are redundant I suspect it will be a net loss for you, but
as I said above you should benchmark it.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Feature Proposal: Connection Pool Optimization - Change the Connection User

[Todd Hubers]

Hi,

I have just joined to start a community consultation process for a
proposal. I just finished the proposal document, I spent time writing a
Problem and Solution section, and I have done quite a bit of upfront
exploration of the code.

See:

   - Google Document with Commenting turned on
   
https://docs.google.com/document/d/1u6mVKEHfKtR80UrMLNYrp5D6cCSW1_arcTaZ9HcAKlw/edit?usp=sharing.
   Feel free to request Edit access.
   - The current PDF version is attached too for archive purposes

I am very keen to make this happen.

Thanks

-- 
--
Todd Hubers


PostgreSQL Change User feature_20211121.pdf
Description: Adobe PDF document

Re: Test::More version

[Tom Lane]

Andrew Dunstan  writes:
> Yeah, so I think at this stage we're just waiting for you to update
> prairiedog and we can make this change.

Oh!  I was intentionally waiting to do that, with the idea of verifying
that the version-detection test works ;-).  I'm prepared to do it as
soon as you push an update and we see prairiedog go red.

regards, tom lane

Re: TOAST - why separate visibility map

[Tom Lane]

Andres Freund  writes:
> On November 19, 2021 12:31:00 PM PST, Tom Lane  wrote:
>> It might be feasible to drop the visibility map for toast tables, though.

> I think it be a bad idea - the VM is used by vacuum to avoid rereading 
> already vacuumed ranges. Loosing that for large toast tables would be bad.

Ah, right.  I was thinking vacuuming depended on the other map fork,
but of course it needs this one.

In short, there are indeed good reasons why it works like this.

regards, tom lane

Re: Feature Proposal: Connection Pool Optimization - Change the Connection User

[Andrey Borodin]

Hi Todd!

> I have just joined to start a community consultation process for a proposal. 
> I just finished the proposal document, I spent time writing a Problem and 
> Solution section, and I have done quite a bit of upfront exploration of the 
> code.
> 
> See:
> 
> * Google Document with Commenting turned on 
> https://docs.google.com/document/d/1u6mVKEHfKtR80UrMLNYrp5D6cCSW1_arcTaZ9HcAKlw/edit?usp=sharing.
>  Feel free to request Edit access.
> * The current PDF version is attached too for archive purposes

I think you are right: the ability to reconnect to the same DB with another 
user would be beneficial for connection poolers.
Currently, PgBouncer\Odyssey maintains a lot of small pools - one for each 
user, while having a big one would be great.
>From my experience, most of the cost of opening a new server connection comes 
>from a fork() and subsequent CoW memory allocations.
And these expenses would be not necessary if we could just send a new Startup 
message after the Terminate ('X') message.
But this effectively would empty out out all backend caches.
Yet the feature seems useful from my PoV.

Best regards, Andrey Borodin.

AW: VS2022: Support Visual Studio 2022 on Windows

[Hans Buschmann]

Hello Daniel,

Thank you for looking into it.

My skills with git are minmal yet and I am working on a correct development 
platform, so sorry for any inconveniances from my side .

When upgraded Microsoft jumped directly from Preview 7 to Preview 7.1 of VS2022 
by skipping the release version of 7.0.

I had to install it on a different machine to test it with the final VS2022 
version from november 8.

On both platforms the build of snapshot from 19.11.2021 is successfull but 
gives the following  warnings which seem not correlated to the proposed patch:

Der Buildvorgang wurde erfolgreich ausgeführt.

"C:\pgdev\postgresql-15devel\pgsql.sln" (Standardziel) (1) ->
"C:\pgdev\postgresql-15devel\postgres.vcxproj" (Standardziel) (2) ->
(ClCompile Ziel) ->
  C:\pgdev\postgresql-15devel\src\backend\access\heap\pruneheap.c(858,18): 
warning C4101: "htup": Unreferenzierte lokale Variable 
[C:\pgdev\postgresql-15devel\postgres.vcxproj]
  C:\pgdev\postgresql-15devel\src\backend\access\heap\pruneheap.c(870,11): 
warning C4101: "tolp": Unreferenzierte lokale Variable 
[C:\pgdev\postgresql-15devel\postgres.vcxproj]

2 Warnung(en)
0 Fehler

(Meaning 2 unreferenced local variables in pruneheap.c)

The build produced .vcxproj files with ToolsVersion="17.0", so it recognized 
the new environment correctly.

I corrected some ommissions in _GetVisualStudioVersion in VSObjectFactory.pm.

Please find attached the corrected patch version v4.

Due to my restricted devlopment environment I appreciate if anybody can test 
the resulting binaries (but MS seems not have changed much on the C Build 
environment internally).

Thanks

Hans Buschmann


0001_support_vs2022_v4.patch
Description: 0001_support_vs2022_v4.patch

Re: Feature Proposal: Connection Pool Optimization - Change the Connection User

[Justin Pryzby]

On Sun, Nov 21, 2021 at 03:11:03AM +1100, Todd Hubers wrote:
> I have just joined to start a community consultation process for a
> proposal. I just finished the proposal document, I spent time writing a
> Problem and Solution section, and I have done quite a bit of upfront
> exploration of the code.
> 
>- Google Document with Commenting turned on
>
> https://docs.google.com/document/d/1u6mVKEHfKtR80UrMLNYrp5D6cCSW1_arcTaZ9HcAKlw/edit?usp=sharing.

The stated goal is to support persistent connections with hundreds or thousands
of different users.

However, this doesn't attempt to address the similar issue if there's hundreds
or thousands of DBs.  Which I don't think could work at all, since switching to
a new DB requires connecting to a new backend process.

You proposed a PQ protocol version of SET ROLE/SET SESSION authorization.
You'd need to make sure that a client connected to the connection pooler cannot
itself run the PQ "set role".  The connection pooler would need to reject the
request (or maybe ignore requests to switch to the same/current user).  Maybe
you'd have two protocol requests "begin switch user" and "end switch to user",
and then the server-side could enforce that "begin switch" is not nested.
Maybe the "begin switch" could return some kind of "nonce" to the connection
pooler, and "end switch" would require the same nonce to be validated.

It'd be interesting to hear if you've tested with postgresql 14, which improves
scalability to larger number of connections.

-- 
Justin

Re: RecoveryInProgress() has critical side effects

[Heikki Linnakangas]

On 17/11/2021 00:04, Andres Freund wrote:

On 2021-11-16 16:30:27 -0500, Robert Haas wrote:

I'm still not entirely clear on whether you prefer v1-0002, v2-0002,
or something else.


I think it basically doesn't matter much. It's such a small piece of the cost
compared to either the cost of a single insert or the ratio between
RedoRecPtr/FPW changes and the number of inserted records.

I guess v2-0002 is mildly simpler, so I very weakly lean towards that.


I was going to argue for v1, because I was worried about generating 
full-page writes unnecessarily. But looking closer, it doesn't do that. 
It will initially think that no full page images are needed, and perform 
the extra loop in XLogInsert() to try again with full page images. 
Similarly in heap_update(), it will fail in the direction of sometimes 
doing the "prefix-suffix compression" unnecessarily, which means a 
little bit of extra CPU work, but it won't affect the WAL record that 
gets written.


So either way is fine, really. That's a bit subtle, though, a comment 
somewhere would be good.


But here's yet another idea: We could initialize RedoRecPtr and 
doPageWrites in InitXLogInsert(). It would seem like a pretty natural 
place for it.


- Heikki

PS. typo in patch v2: s/prepard/prepared/

Re: Feature Proposal: Connection Pool Optimization - Change the Connection User

[Tom Lane]

Justin Pryzby  writes:
> On Sun, Nov 21, 2021 at 03:11:03AM +1100, Todd Hubers wrote:
>> - Google Document with Commenting turned on
>> https://docs.google.com/document/d/1u6mVKEHfKtR80UrMLNYrp5D6cCSW1_arcTaZ9HcAKlw/edit?usp=sharing.

> You proposed a PQ protocol version of SET ROLE/SET SESSION authorization.
> You'd need to make sure that a client connected to the connection pooler 
> cannot
> itself run the PQ "set role".

It's not really apparent to me how this mechanism wouldn't be a giant
security hole.  In particular, I see no meaningful difference between
granting the proposed "impersonate" privilege and granting superuser.
You could restrict it to not allow impersonating superusers, which'd
make it a little better, but what of all the predefined roles we keep
inventing that have privileges we don't want to be accessible to Joe
User?  I think by the time you got to something where "impersonate"
wasn't a security hole, it would be pretty much equivalent to SET ROLE,
ie you could only impersonate users you'd been specifically given the
privlege for.

It also seems like this is transferring the entire responsibility for
user authentication onto the middleware, with the server expected to
just believe that the session should now be allowed to act as user X.
Again, that seems more like a security problem than a good design.

Another issue is that the proposed implementation seems pretty far
short of being an invisible substitute for actually logging in as
the target user.  For starters, what of instantiating ALTER ROLE SET
parameter values that apply to the target user, and getting rid of
such settings that applied to the original user ID?  How would this
interact with the "on login" triggers that people keep asking for?

Also, you'd still have to do DISCARD ALL (at least) when switching
users, or else integrate all that cache-flushing right into the
switching mechanism.  So I'm not entirely convinced about how big
the performance benefits are compared to a fresh session.

One more point is that the proposed business about 

* ImpersonateDatabaseUser will either succeed silently (0-RTT), or
  fail. Upon failure, no further commands will be processed until
  ImpersonateDatabaseUser succeeds.

seems to require adding a huge amount of complication on the server side,
and complication in the protocol spec itself, to save a rather minimal
amount of complication in the middleware.  Why can't we just say that
a failed "impersonate" command leaves the session in the same state
as before, and it's up to the pooler to do something about it?  We are
in any case trusting the pooler not to send commands from user A to
a session logged in as user B.

regards, tom lane

PS: I wonder how we test such a feature meaningfully without
incorporating a pooler right into the Postgres tree.  I don't
want to do that, for sure.

Re: Improving psql's \password command

[Tom Lane]

"Bossart, Nathan"  writes:
> On 11/19/21, 9:17 AM, "Tom Lane"  wrote:
>> Hmm, initdb's prompt-for-superuser-password might need it.

> I'm able to cancel the superuser password prompt in initdb already.
> It looks like the signal handlers aren't set up until after
> get_su_pwd().

Right; I misread that code in an overly hasty scan.

> I did find some missing control-C handling in
> pg_receivewal/pg_recvlogical, though.  Attached is a patch for those.

Meh ... I'm inclined to fix those programs by just moving their pqsignal
calls down to after their initial GetConnection calls, as attached.
This'd be simple enough to back-patch, for one thing.

It could be argued that this doesn't provide a nice experience if
(a) somebody changes your password mid-run and (b) you actually
need to make a new connection for some reason and (c) you want
to give up at that point instead of putting in the new password.
But I doubt it's worth so much extra complication to address that
edge case.  We've had about zero field complaints about the existing
behavior in those programs, so the cost/benefit ratio seems poor.

regards, tom lane

PS: I noticed that StreamLogicalLog leaks its query buffer if
GetConnection fails.  Probably not very exciting, but we might
as well fix that, as included below.
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 23cf5f8ec7..4b1439be90 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -917,10 +917,6 @@ main(int argc, char **argv)
 		close_destination_dir(dir, basedir);
 	}
 
-#ifndef WIN32
-	pqsignal(SIGINT, sigint_handler);
-#endif
-
 	/*
 	 * Obtain a connection before doing anything.
 	 */
@@ -930,6 +926,14 @@ main(int argc, char **argv)
 		exit(1);
 	atexit(disconnect_atexit);
 
+	/*
+	 * Trap signals.  (Don't do this until after the initial password prompt,
+	 * if one is needed, in GetConnection.)
+	 */
+#ifndef WIN32
+	pqsignal(SIGINT, sigint_handler);
+#endif
+
 	/*
 	 * Run IDENTIFY_SYSTEM to make sure we've successfully have established a
 	 * replication connection and haven't connected using a database specific
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f235d6fecf..13319cf0d3 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -216,8 +216,6 @@ StreamLogicalLog(void)
 	output_written_lsn = InvalidXLogRecPtr;
 	output_fsync_lsn = InvalidXLogRecPtr;
 
-	query = createPQExpBuffer();
-
 	/*
 	 * Connect in replication mode to the server
 	 */
@@ -236,6 +234,7 @@ StreamLogicalLog(void)
 	replication_slot);
 
 	/* Initiate the replication stream at specified location */
+	query = createPQExpBuffer();
 	appendPQExpBuffer(query, "START_REPLICATION SLOT \"%s\" LOGICAL %X/%X",
 	  replication_slot, LSN_FORMAT_ARGS(startpos));
 
@@ -932,16 +931,9 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
-
-#ifndef WIN32
-	pqsignal(SIGINT, sigint_handler);
-	pqsignal(SIGHUP, sighup_handler);
-#endif
-
 	/*
-	 * Obtain a connection to server. This is not really necessary but it
-	 * helps to get more precise error messages about authentication, required
-	 * GUC parameters and such.
+	 * Obtain a connection to server.  Notably, if we need a password, we want
+	 * to collect it from the user immediately.
 	 */
 	conn = GetConnection();
 	if (!conn)
@@ -949,6 +941,15 @@ main(int argc, char **argv)
 		exit(1);
 	atexit(disconnect_atexit);
 
+	/*
+	 * Trap signals.  (Don't do this until after the initial password prompt,
+	 * if one is needed, in GetConnection.)
+	 */
+#ifndef WIN32
+	pqsignal(SIGINT, sigint_handler);
+	pqsignal(SIGHUP, sighup_handler);
+#endif
+
 	/*
 	 * Run IDENTIFY_SYSTEM to make sure we connected using a database specific
 	 * replication connection.

Re: Test::More version

[Andrew Dunstan]

On 11/20/21 11:14, Tom Lane wrote:
> Andrew Dunstan  writes:
>> Yeah, so I think at this stage we're just waiting for you to update
>> prairiedog and we can make this change.
> Oh!  I was intentionally waiting to do that, with the idea of verifying
> that the version-detection test works ;-).  I'm prepared to do it as
> soon as you push an update and we see prairiedog go red.


Ah, ok. Pushed.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Improving psql's \password command

[Bossart, Nathan]

On 11/20/21, 1:58 PM, "Tom Lane"  wrote:
> "Bossart, Nathan"  writes:
>> I did find some missing control-C handling in
>> pg_receivewal/pg_recvlogical, though.  Attached is a patch for those.
>
> Meh ... I'm inclined to fix those programs by just moving their pqsignal
> calls down to after their initial GetConnection calls, as attached.
> This'd be simple enough to back-patch, for one thing.
>
> It could be argued that this doesn't provide a nice experience if
> (a) somebody changes your password mid-run and (b) you actually
> need to make a new connection for some reason and (c) you want
> to give up at that point instead of putting in the new password.
> But I doubt it's worth so much extra complication to address that
> edge case.  We've had about zero field complaints about the existing
> behavior in those programs, so the cost/benefit ratio seems poor.

Yeah, I was looking for a way to simplify this, too.  Your approach
seems reasonable enough to me.

Nathan

Re: logical decoding/replication: new functions pg_ls_logicaldir and pg_ls_replslotdir

[Michael Paquier]

On Sat, Nov 20, 2021 at 12:29:51AM +, Bossart, Nathan wrote:
> On 11/17/21, 11:39 PM, "Bharath Rupireddy" 
>  wrote:
>> Please review the attached v2.
> 
> LGTM.  I've marked this one as ready-for-committer.

One issue that I have with this patch is that there are zero
regression tests.  Could you add a couple of things in
misc_functions.sql (for the negative tests perhaps) or
contrib/test_decoding/, taking advantage of places where slots are
already created?  You may want to look after the non-superuser case
where the calls should fail, and the second case where a role is part
of pg_monitor where the call succeeds.  Note that any roles created in
the tests have to be prefixed with "regress_".

+   snprintf(path, sizeof(path), "%s/%s", "pg_replslot", slotname);
+   return pg_ls_dir_files(fcinfo, path, false);
"pg_replslot" could be part of the third argument here.  There is no
need to separate it.

+ordinary file in the server's pg_logical/mappings directory.
Paths had better have  markups around them, no?
--
Michael


signature.asc
Description: PGP signature

Re: VS2022: Support Visual Studio 2022 on Windows

[Michael Paquier]

On Sat, Nov 20, 2021 at 05:54:30PM +, Hans Buschmann wrote:
> My skills with git are minmal yet and I am working on a correct
> development platform, so sorry for any inconveniances from my side.

No need to worry here.  We all learn all the time.  I have been able
to apply your patch with a "patch -p2", which is fine enough.  If you
want to generate cleaner diffs, you could use a "git diff" or a "git
format-patch".  Folks around here rely on those commands heavily when
generating patches.

> On both platforms the build of snapshot from 19.11.2021 is
> successfull but gives the following  warnings which seem not
> correlated to the proposed patch:

That's fine by me.

> Der Buildvorgang wurde erfolgreich ausgeführt.
> 
> "C:\pgdev\postgresql-15devel\pgsql.sln" (Standardziel) (1) ->
> "C:\pgdev\postgresql-15devel\postgres.vcxproj" (Standardziel) (2) ->
> (ClCompile Ziel) ->
>   C:\pgdev\postgresql-15devel\src\backend\access\heap\pruneheap.c(858,18): 
> warning C4101: "htup": Unreferenzierte lokale Variable 
> [C:\pgdev\postgresql-15devel\postgres.vcxproj]
>   C:\pgdev\postgresql-15devel\src\backend\access\heap\pruneheap.c(870,11): 
> warning C4101: "tolp": Unreferenzierte lokale Variable 
> [C:\pgdev\postgresql-15devel\postgres.vcxproj]
> 
> 2 Warnung(en)
> 0 Fehler
> 
> (Meaning 2 unreferenced local variables in pruneheap.c)

Those warnings are knows.  A commit from Peter G is at the origin of
that but nothing has been done about these yet:
https://www.postgresql.org/message-id/yyttuyykpvxef...@paquier.xyz

So don't worry about that :)

Glad to see that we should have nothing to do about locales this
time.  I have not tested, but I think that you covering all the areas
that need a refresh here.  Nice work.

+   # The version of nmake bundled in Visual Studio 2022 is greater
+   # than 14.30 and less than 14.40.  And the version number is
+   # actually 17.00.
+   elsif (
+   ($visualStudioVersion ge '14.30' && $visualStudioVersion lt '14.40')
+   || $visualStudioVersion eq '17.00')
+   {
+   return new VS2022Solution(@_);
+   }
Wow, really?  MSVC has not yet simplified their version numbering with
nmake.

+VC2017Project,VC2019Project or VC2022Project from MSBuildProject.pm) to it.
Nit: you should use a space when listing elements in a comma-separated
list.

-  method for compressing table or WAL data. Binaries and source can be
+  method for compressing the table data. Binaries and source can be
Diff unrelated to your patch.   

I'll double-check your patch later, but that looks rather good to me.
Will try to apply and back-patch, and it would be better to check the
version numbers assigned in the patch, as well.
--
Michael


signature.asc
Description: PGP signature

Re: Feature Proposal: Connection Pool Optimization - Change the Connection User

[Todd Hubers]

Hi Tom, Justin, and Andrey,

Thanks everybody for your feedback so far! I agree, there are a few
unknowns for the design and impact and there are many details to iron out.

*Benchmarking* - Overall I think it's best to explore improvements with
benchmarking. The key goal of this proposal pertains to performance:
"fast". That means benchmarking is essential in the design phase, to ensure
there are measurable improvements, and reward for effort. This should also
primarily influence the selection of Solution Option (of which there are 6
or 7 listed). *I have added the proposed benchmarks to the document.
*

*1) Andrey said*: "And these expenses would be not necessary *if we could
just send a new Startup message after the Terminate ('X') message*." -
*Answer*: I have added the "Terminate/Restartup" *benchmark in the document
*accordingly.

*2) Justin said:* "However, this doesn't attempt to address the similar
issue *if there's hundreds or thousands of DBs*.  Which I don't think could
work at all, since switching to a new DB requires connecting to a new
backend process."

Good point. I do have some loose thinking around that. I do have some
comments throughout the proposal document to *tackle that in the future*.
As you point out, user switching seems to be a simpler well-trodden path.
For now, my proposal is that the middleware can assert a database change,
but that should create an error for now. In the future, it might be a
supported capability.

I have also added database-switching as a consideration for the
*benchmarking*.

*3) Justin said*: "You'd need to make sure that a client connected to the
connection pooler cannot itself run the PQ "set role".  *The connection
pooler would need to reject the request *(or maybe ignore requests to
switch to the same/current user)." - *Answer: Agreed*.

*4) Justin said:* "Maybe you'd have two protocol requests "begin switch
user" and "end switch to user", and then the server-side could enforce that
"begin switch" is not nested. Maybe the "begin switch" could return some
kind of "nonce" to the connection pooler, and "end switch" *would require
the same nonce* to be validated."

Agreed, something like this should be suitable. For performance, *I would
prefer to not use a Nonce* because that would not be a 0-RTT approach. The
PQ level ImpersonateDatabaseUser is effectively the "begin". I don't think
an "end" is necessary, because it is expected to switch the user very
often. The next "begin" is effectively ending the last Impersonation. The
middleware could switch back to its own username, but as per my draft
proposal, that user should only be allowed (as a connection bound
authorisation for ImpersonateDatabaseUser) to impersonate and nothing else.

And then there are yet 5 other solution options which have an explicit
Begin/End pattern.

*5) Justin said:* "It'd be interesting to hear if you've tested with
postgresql 14, which improves scalability to larger number of connections."

Agreed. I will include a baseline in the benchmarks. Regardless of
improvements, it won't be infinite and there will still be a need to
"enable" pooling where there are MAX_CONNECTIONS users using the system.

*6) Tom said:* "It's not really apparent to me how this mechanism wouldn't
be a giant security hole."

I have added your important concerns to a Security Considerations section
of the document:

   - "What is the difference between granting the proposed "impersonate"
   privilege and granting superuser?"
   - "Is this merely transferring the entire responsibility for user
   authentication onto the middleware, with the server expected to just
   believe that the session should now be allowed to act as user X?"

*7) Tom said:* "I think by the time you got to something where
"impersonate" wasn't a security hole,* it would be pretty much equivalent
to SET ROLE*"

*I don't think so*. This is already contemplated in the draft. See Solution
Option 4. SET ROLE comes with RESET ROLE. The frontend client could call
RESET ROLE then change to whatever role they like. That means that feature
is not currently suitable to be used for the context of this proposal.

*8) Tom said:* "Another issue is that the proposed implementation seems
pretty far short of being an *invisible substitute for actually logging in*
as the target user"

*The goal* of this proposal is performance and *to enable shared connection
pooling*. Direct logging in doesn't allow the reuse of existing connections
in the pool.

*9) Tom said:* "For starters, what of *instantiating ALTER ROLE SET*
parameter values that apply to the target user, and getting rid of such
settings that applied to the original user ID?"

I think you are suggesting to modify the already-logged-in user. That's
interesting. However, *many systems audit the logged in user by username* -
who they are. Furthermore, the modification of user privileges would not
help to e

Re: logical decoding/replication: new functions pg_ls_logicaldir and pg_ls_replslotdir

[Bharath Rupireddy]

On Sun, Nov 21, 2021 at 6:58 AM Michael Paquier  wrote:
>
> On Sat, Nov 20, 2021 at 12:29:51AM +, Bossart, Nathan wrote:
> > On 11/17/21, 11:39 PM, "Bharath Rupireddy" 
> >  wrote:
> >> Please review the attached v2.
> >
> > LGTM.  I've marked this one as ready-for-committer.
>
> One issue that I have with this patch is that there are zero
> regression tests.  Could you add a couple of things in
> misc_functions.sql (for the negative tests perhaps) or
> contrib/test_decoding/, taking advantage of places where slots are
> already created?  You may want to look after the non-superuser case
> where the calls should fail, and the second case where a role is part
> of pg_monitor where the call succeeds.  Note that any roles created in
> the tests have to be prefixed with "regress_".

I don't think we need to go far to contrib/test_decoding/, even if we
add it there we can't test it for the outputs of these functions, so
I've added the tests in misc_functinos.sql itself.

> +   snprintf(path, sizeof(path), "%s/%s", "pg_replslot", slotname);
> +   return pg_ls_dir_files(fcinfo, path, false);
> "pg_replslot" could be part of the third argument here.  There is no
> need to separate it.

Done.

> +ordinary file in the server's pg_logical/mappings directory.
> Paths had better have  markups around them, no?

Done.

Attached v3 patch, please review it further.

Regards,
Bharath Rupireddy.


v3-0001-Add-pg_ls_logicalsnapdir-pg_ls_logicalmapdir-pg_l.patch
Description: Binary data

Re: Parallel Full Hash Join

[Justin Pryzby]

On Wed, Nov 17, 2021 at 01:45:06PM -0500, Melanie Plageman wrote:
> On Sat, Nov 6, 2021 at 11:04 PM Justin Pryzby  wrote:
> >
> > > Rebased patches attached. I will change status back to "Ready for 
> > > Committer"
> >
> > The CI showed a crash on freebsd, which I reproduced.
> > https://cirrus-ci.com/task/5203060415791104
> >
> > The crash is evidenced in 0001 - but only ~15% of the time.
> >
> > I think it's the same thing which was committed and then reverted here, so
> > maybe I'm not saying anything new.
> >
> > https://commitfest.postgresql.org/33/3031/
> > https://www.postgresql.org/message-id/flat/20200929061142.ga29...@paquier.xyz
> 
> Yes, this looks like that issue.
> 
> I've attached a v8 set with the fix I suggested in [1] included.
> (I added it to 0001).

This is still crashing :(
https://cirrus-ci.com/task/6738329224871936
https://cirrus-ci.com/task/4895130286030848

-- 
Justin

Re: [RFC] ASOF Join

[Ilya Anfimov]

On Thu, Nov 18, 2021 at 05:11:16PM +0300, Alexander Kuzmenkov wrote:
> Hi hackers,
> 
> There was some interest in implementing ASOF joins in Postgres, see
> e.g. this prototype patch by Konstantin Knizhnik:
> https://www.postgresql.org/message-id/flat/bc494762-26bd-b100-e1f9-a97901ddad57%40postgrespro.ru
> I't like to discuss the possible ways of implementation, if there is
> still any interest in that.
> 
> 
> Introduction
> 
> ASOF join is often used to work with time series data such as stock
> quotes or IoT sensors. It is an interpolation where we want to relate
> two different time series measured at different points in time. For
> each value of the first time series, we take the most recent value of
> the second.

DISCLAIMER:  I  am both seeing this first time and I don't have a
good understanding of the PosgreSQL development practices.

 But at a first glance the syntax looks like  pure  evil.  I  see
that  this  is somewhat common as of now (clickhouse, your refer-
enced questdb, quasar db) -- but it's bad anyway.  And not really
a standard.

 This  introduces  a  new  keyword to the ridiculous list of key-
words.
 The syntax loosely defines what preference it wants, by extract-
ing some vague set of ordering operators from the join condition.
 Also,  only  one  asof value is allowed (there may be more of it
sometimes).

 Perhaps, if we've got to some syntax -- than something like
 ORDER  BY  ... LIMIT 
in  joins,  just before the ON join_condition or USING() could be
much better.
 This allows to
   1) Easily separate just conditions from the likehood ones.  No
deep  analysis  of  a  condition expression necessary. No need to
have another list of all the possible ranking functions and oper-
ators.
   2) Have ordering preference for many ranking conditions.  Like
ORDER BY secondary_field IS NOT NULL DESC, time_difference, reli-
ability
   3) Have more than one row returned for a joined table.


 But  anyways  this  looks  like  just a syntactic sugar. LATERAL
JOINS should logically work just fine.  Any  optimisation  should
deal with the LATERAL syntax style anyway.


> 
> Besides an inequality condition on timestamp, such join can also have
> equality conditions on other columns. For example, this query joins
> two tables that contain bids and asks, finding the most recent task
> for each bid for given financial instrument:
> 
> ```sql
> SELECT bids.ts timebid, bid, ask
> FROM bids
> ASOF JOIN asks ON bid.instrument = ask.instrument
> AND ask.ts <= bid.ts;
> ```
> 
> Semantically, this is equivalent to the following correlated subquery:
> ```sql
> SELECT bids.ts timebid, bid, ask
> FROM bids,
> LATERAL (select * from asks
> WHERE asks.instrument = bids.instrument AND asks.ts <= bids.ts
> ORDER BY ts DESC LIMIT 1) t;
> ```
> This form is useful to think about which optimizations we can perform
> with an ASOF join, how it behaves with respect to other joins, and so
> on.
> 
> QuestDB has some good docs on this with more examples:
> https://questdb.io/docs/reference/sql/join/#asof-join
> 
> 
> What Conditions Work with ASOF Join
> 
> Conditions for an ASOF join consist of one inequality condition (>=
> etc), and optionally a number of equality conditions. All these
> conditions must be "mergejoinable" in PG terms -- they must belong to
> a btree operator family, which means there is a sorting operator that
> corresponds to the condition, which means we can perform a merge join.
> They also must support hashing because we'll probably need both
> sorting and hashing for implementation (see below). This holds for the
> usual data types like numeric. It is natural to think of the
> inequality column as "time", but technically it can be any column,
> even a string one, w/o changing the algorithm.
> 
> 
> Join variants
> 
> The purpose of ASOF join is interpolation of one time series to match
> another, so it is natural to think of it as an INNER join. The outer
> variants might be less useful. Technically, it is easy to extend it to
> LEFT ASOF JOIN, where we would output nulls for the right hand columns
> if we haven???t yet seen a match. RIGHT and FULL variants also make
> sense, but the implementation may be impossible, depending on the
> algorithm -- merge and hash joins can support these variants, but the
> nested loop cannot.
> 
> 
> Use in Combination with Normal Joins
> 
> The difference of ASOF join from normal join is that for the
> inequality condition, it does not output all the rows that match it,
> but only the most recent one. We can think of it as first performing a
> normal join and then applying a filter that selects the latest right
> hand row. Which row is the "latest" depends on the entire set of rows
> that match the join conditions (same as with LIMIT). This means that
> the result of ASOF join may depend on the place in the join tree where
> it is evaluated, because other joins may remove some rows. Similar to
> outer joins, we must respect the user-specified jo

Re: [RFC] ASOF Join

[Todd Hubers]

>But  anyways  this  looks  like  just a syntactic sugar. LATERAL
>JOINS should logically work just fine.  Any  optimisation  should
>deal with the LATERAL syntax style anyway.

Agreed.

However, if a rewrite is implemented, it then becomes encoded into
PostgreSQL code what ASOF maps to. Anyone who wants to use ASOF can use it,
and anyone who wants to directly use LATERAL can do so also.

On Sun, 21 Nov 2021 at 15:54, Ilya Anfimov  wrote:

> On Thu, Nov 18, 2021 at 05:11:16PM +0300, Alexander Kuzmenkov wrote:
> > Hi hackers,
> >
> > There was some interest in implementing ASOF joins in Postgres, see
> > e.g. this prototype patch by Konstantin Knizhnik:
> >
> https://www.postgresql.org/message-id/flat/bc494762-26bd-b100-e1f9-a97901ddad57%40postgrespro.ru
> > I't like to discuss the possible ways of implementation, if there is
> > still any interest in that.
> >
> >
> > Introduction
> >
> > ASOF join is often used to work with time series data such as stock
> > quotes or IoT sensors. It is an interpolation where we want to relate
> > two different time series measured at different points in time. For
> > each value of the first time series, we take the most recent value of
> > the second.
>
> DISCLAIMER:  I  am both seeing this first time and I don't have a
> good understanding of the PosgreSQL development practices.
>
>  But at a first glance the syntax looks like  pure  evil.  I  see
> that  this  is somewhat common as of now (clickhouse, your refer-
> enced questdb, quasar db) -- but it's bad anyway.  And not really
> a standard.
>
>  This  introduces  a  new  keyword to the ridiculous list of key-
> words.
>  The syntax loosely defines what preference it wants, by extract-
> ing some vague set of ordering operators from the join condition.
>  Also,  only  one  asof value is allowed (there may be more of it
> sometimes).
>
>  Perhaps, if we've got to some syntax -- than something like
>  ORDER  BY  ... LIMIT
> in  joins,  just before the ON join_condition or USING() could be
> much better.
>  This allows to
>1) Easily separate just conditions from the likehood ones.  No
> deep  analysis  of  a  condition expression necessary. No need to
> have another list of all the possible ranking functions and oper-
> ators.
>2) Have ordering preference for many ranking conditions.  Like
> ORDER BY secondary_field IS NOT NULL DESC, time_difference, reli-
> ability
>3) Have more than one row returned for a joined table.
>
>
>  But  anyways  this  looks  like  just a syntactic sugar. LATERAL
> JOINS should logically work just fine.  Any  optimisation  should
> deal with the LATERAL syntax style anyway.
>
>
> >
> > Besides an inequality condition on timestamp, such join can also have
> > equality conditions on other columns. For example, this query joins
> > two tables that contain bids and asks, finding the most recent task
> > for each bid for given financial instrument:
> >
> > ```sql
> > SELECT bids.ts timebid, bid, ask
> > FROM bids
> > ASOF JOIN asks ON bid.instrument = ask.instrument
> > AND ask.ts <= bid.ts;
> > ```
> >
> > Semantically, this is equivalent to the following correlated subquery:
> > ```sql
> > SELECT bids.ts timebid, bid, ask
> > FROM bids,
> > LATERAL (select * from asks
> > WHERE asks.instrument = bids.instrument AND asks.ts <= bids.ts
> > ORDER BY ts DESC LIMIT 1) t;
> > ```
> > This form is useful to think about which optimizations we can perform
> > with an ASOF join, how it behaves with respect to other joins, and so
> > on.
> >
> > QuestDB has some good docs on this with more examples:
> > https://questdb.io/docs/reference/sql/join/#asof-join
> >
> >
> > What Conditions Work with ASOF Join
> >
> > Conditions for an ASOF join consist of one inequality condition (>=
> > etc), and optionally a number of equality conditions. All these
> > conditions must be "mergejoinable" in PG terms -- they must belong to
> > a btree operator family, which means there is a sorting operator that
> > corresponds to the condition, which means we can perform a merge join.
> > They also must support hashing because we'll probably need both
> > sorting and hashing for implementation (see below). This holds for the
> > usual data types like numeric. It is natural to think of the
> > inequality column as "time", but technically it can be any column,
> > even a string one, w/o changing the algorithm.
> >
> >
> > Join variants
> >
> > The purpose of ASOF join is interpolation of one time series to match
> > another, so it is natural to think of it as an INNER join. The outer
> > variants might be less useful. Technically, it is easy to extend it to
> > LEFT ASOF JOIN, where we would output nulls for the right hand columns
> > if we haven???t yet seen a match. RIGHT and FULL variants also make
> > sense, but the implementation may be impossible, depending on the
> > algorithm -- merge and hash joins can support these variants, but the
> > nested loop cannot.
> >
> >
> > Use in Combination