from:"Gurjeet"

When reviewing a recently committed patch [1] I noticed the odd usage
of a format specifier:

+   libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
+   "load_balance_hosts",
+   conn->load_balance_hosts);

The oddity is that the first %s is unnecessary, since the value we
want there is a constant. Typically a format specifier used to get the
value stored in a variable.

Upon closer look, it looks like this is a common pattern in
fe-connect.c; there are many places where a %s format specifier is
being used in the format sting, where the name of the parameter would
have sufficed.

Upon some research, the only explanation I could come up with was that
this pattern of specifying error messages helps with message
translations. This way there's just one message to be translated. For
example:

.../libpq/po/es.po-#: fe-connect.c:1268 fe-connect.c:1294
fe-connect.c:1336 fe-connect.c:1345
.../libpq/po/es.po-#: fe-connect.c:1378 fe-connect.c:1422
.../libpq/po/es.po-#, c-format
.../libpq/po/es.po:msgid "invalid %s value: \"%s\"\n"

There's just one exception to this pattern, though.

> libpq_append_conn_error(conn, "invalid require_auth method: \"%s\"",
> method);

So, to make it consistent throughout the file, we should either
replace all such %s format specifiers with the respective strings, or
use the same pattern for the message string used for require_method,
as well. Attached patch [2] does the former, and patch [3] does the
latter.

Pick your favorite one.

[1]: Support connection load balancing in libpq
7f5b19817eaf38e70ad1153db4e644ee9456853e

[2]: Replace placeholders with known strings
v1-0001-Replace-placeholders-with-known-strings.patch

[3]: Make require_auth error message similar to surrounding messages
v1-0001-Make-require_auth-error-message-similar-to-surrou.patch

Best regards,
Gurjeet http://Gurje.et
Postgres Contributors Team, http://aws.amazon.com


v1-0001-Replace-placeholders-with-known-strings.patch
Description: Binary data


v1-0001-Make-require_auth-error-message-similar-to-surrou.patch
Description: Binary data

Re: Make message strings in fe-connect.c consistent

On Thu, Apr 20, 2023 at 9:31 PM Tom Lane  wrote:
>
> Gurjeet Singh  writes:
> > When reviewing a recently committed patch [1] I noticed the odd usage
> > of a format specifier:
>
> > +   libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
> > +   "load_balance_hosts",
> > +   conn->load_balance_hosts);
>
> > The oddity is that the first %s is unnecessary, since the value we
> > want there is a constant. Typically a format specifier used to get the
> > value stored in a variable.
>
> This is actually intentional, on the grounds that it reduces the
> number of format strings that require translation.

That's the only reason I too could come up with.

> > There's just one exception to this pattern, though.
>
> >> libpq_append_conn_error(conn, "invalid require_auth method: \"%s\"",
> >> method);
>
> Yup, this one did not get the memo.

That explains why I could not find any translation for this error message.

Best regards,
Gurjeet http://Gurje.et
Postgres Contributors Team, http://aws.amazon.com

Minor code de-duplication in fe-connect.c

Commit [1] implements Fisher-Yates shuffling algorithm to shuffle
connection addresses, in two places.

The attached patch moves the duplicated code to a function, and calls
it in those 2 places.

[1]: Support connection load balancing in libpq
7f5b19817eaf38e70ad1153db4e644ee9456853e

Best regards,
Gurjeet https://Gurje.et
Postgres Contributors Team, http://aws.amazon.com


v0-0001-Reduce-code-duplication-in-fe-connect.c.patch
Description: Binary data

Reorder connection markers in libpq TAP tests

Commit 7f5b198 introduced TAP tests that use string literals to mark
the presence of a query in server logs. For no explicable reason, the
tests with the marker 'connect2' occur before the tests that use
'connect1' marker.

The attached patch swaps the connection marker strings so that a
reader doesn't have to spend extra deciphering why 'connect2' tests
appear before 'connect1' tests.

Best regards,
Gurjeet https://Gurje.et
Postgres Contributors Team, http://aws.amazon.com


v0-0001-Reorder-connection-markers-in-TAP-tests.patch
Description: Binary data

Re: Reorder connection markers in libpq TAP tests

On Thu, Apr 20, 2023 at 11:00 PM Gurjeet Singh  wrote:
>
> Commit 7f5b198 introduced TAP tests that use string literals to mark
> the presence of a query in server logs. For no explicable reason, the
> tests with the marker 'connect2' occur before the tests that use
> 'connect1' marker.
>
> The attached patch swaps the connection marker strings so that a
> reader doesn't have to spend extra deciphering why 'connect2' tests
> appear before 'connect1' tests.

Please see attached v2 of the patch. It now includes same fix in
another TAP tests file.


Best regards,
Gurjeet https://Gurje.et
Postgres Contributors Team, http://aws.amazon.com
From 8e785d5b440f17f7788c722582b2613e092ca18c Mon Sep 17 00:00:00 2001
From: Gurjeet Singh 
Date: Thu, 20 Apr 2023 22:49:00 -0700
Subject: [PATCH v2] Reorder connection markers in TAP tests

Commit 7f5b198 introduced TAP tests that use string literals to mark the
presence of a query in server logs. For no explicable reason, the tests
with the marker 'connect2' occur before the tests that use 'connect1'
marker.

So swap the connection marker strings so that a reader doesn't have to
spend extra effort deciphering why 'connect2' tests appear before
'connect1' tests.
---
 .../libpq/t/003_load_balance_host_list.pl  | 12 ++--
 src/interfaces/libpq/t/004_load_balance_dns.pl | 14 +++---
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/interfaces/libpq/t/003_load_balance_host_list.pl b/src/interfaces/libpq/t/003_load_balance_host_list.pl
index 6963ef3849..7f1bb0c5bc 100644
--- a/src/interfaces/libpq/t/003_load_balance_host_list.pl
+++ b/src/interfaces/libpq/t/003_load_balance_host_list.pl
@@ -36,8 +36,8 @@ $node1->connect_fails(
 # load_balance_hosts=disable should always choose the first one.
 $node1->connect_ok("host=$hostlist port=$portlist load_balance_hosts=disable",
 	"load_balance_hosts=disable connects to the first node",
-	sql => "SELECT 'connect2'",
-	log_like => [qr/statement: SELECT 'connect2'/]);
+	sql => "SELECT 'connect1'",
+	log_like => [qr/statement: SELECT 'connect1'/]);
 
 # Statistically the following loop with load_balance_hosts=random will almost
 # certainly connect at least once to each of the nodes. The chance of that not
@@ -45,12 +45,12 @@ $node1->connect_ok("host=$hostlist port=$portlist load_balance_hosts=disable",
 foreach my $i (1 .. 50) {
 	$node1->connect_ok("host=$hostlist port=$portlist load_balance_hosts=random",
 		"repeated connections with random load balancing",
-		sql => "SELECT 'connect1'");
+		sql => "SELECT 'connect2'");
 }
 
-my $node1_occurences = () = $node1->log_content() =~ /statement: SELECT 'connect1'/g;
-my $node2_occurences = () = $node2->log_content() =~ /statement: SELECT 'connect1'/g;
-my $node3_occurences = () = $node3->log_content() =~ /statement: SELECT 'connect1'/g;
+my $node1_occurences = () = $node1->log_content() =~ /statement: SELECT 'connect2'/g;
+my $node2_occurences = () = $node2->log_content() =~ /statement: SELECT 'connect2'/g;
+my $node3_occurences = () = $node3->log_content() =~ /statement: SELECT 'connect2'/g;
 
 my $total_occurences = $node1_occurences + $node2_occurences + $node3_occurences;
 
diff --git a/src/interfaces/libpq/t/004_load_balance_dns.pl b/src/interfaces/libpq/t/004_load_balance_dns.pl
index d9b382dba9..b07bc9c074 100644
--- a/src/interfaces/libpq/t/004_load_balance_dns.pl
+++ b/src/interfaces/libpq/t/004_load_balance_dns.pl
@@ -9,7 +9,7 @@ use Test::More;
 if ($ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
 {
 	plan skip_all =>
-	  'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+	  'Potentially unsafe test; load_balance not enabled in PG_TEST_EXTRA';
 }
 
 # This tests loadbalancing based on a DNS entry that contains multiple records
@@ -78,8 +78,8 @@ $node3->start();
 # load_balance_hosts=disable should always choose the first one.
 $node1->connect_ok("host=pg-loadbalancetest port=$port load_balance_hosts=disable",
 	"load_balance_hosts=disable connects to the first node",
-	sql => "SELECT 'connect2'",
-	log_like => [qr/statement: SELECT 'connect2'/]);
+	sql => "SELECT 'connect1'",
+	log_like => [qr/statement: SELECT 'connect1'/]);
 
 
 # Statistically the following loop with load_balance_hosts=random will almost
@@ -88,12 +88,12 @@ $node1->connect_ok("host=pg-loadbalancetest port=$port load_balance_hosts=disabl
 foreach my $i (1 .. 50) {
 	$node1->connect_ok("host=pg-loadbalancetest port=$port load_balance_hosts=ran

Re: Minor code de-duplication in fe-connect.c

2023-04-21 Thread Gurjeet Singh

On Fri, Apr 21, 2023 at 7:47 AM Robert Haas  wrote:
>
> On Fri, Apr 21, 2023 at 8:25 AM Daniel Gustafsson  wrote:
> > The reason I left it like this when reviewing and committing is that I 
> > think it
> > makes for more readable code.  The amount of lines saved is pretty small, 
> > and
> > "shuffle" isn't an exact term so by reading the code it isn't immediate 
> > clear
> > what such a function would do.  By having the shuffle algorithm where it's 
> > used
> > it's clear what the code does and what the outcome is.  If others disagree I
> > can go ahead and refactor of course, but I personally would not deem it a 
> > net
> > win in code quality.
>
> I think we should avoid nitpicking stuff like this. I likely would
> have used a subroutine if I'd done it myself, but I definitely
> wouldn't have submitted a patch to change whatever the last person did
> without some tangible reason for so doing.

Code duplication, and the possibility that a change in one location
(bugfix or otherwise) does not get applied to the other locations, is
a good enough reason to submit a patch, IMHO.

> It's not a good use of
> reviewer and committer time to litigate things like this.

Postgres has a very high bar for code quality, and for documentation.
It is a major reason that attracts people to, and keeps them in the
Postgres ecosystem, as users and contributors. If anything, we should
encourage folks to point out such inconsistencies in code and docs
that keep the quality high.

This is not a attack on any one commit, or author/committer of the
commit; sorry if it appeared that way. I was merely reviewing the
commit that introduced a nice libpq feature. This patch is merely a
minor improvement to the code that I think deserves a consideration.
It's not a litigation, by any means.

Best regards,
Gurjeet https://Gurje.et
Postgres Contributors Team, http://aws.amazon.com

Re: Correct the documentation for work_mem

2023-04-21 Thread Gurjeet Singh

On Fri, Apr 21, 2023 at 10:15 AM Tom Lane  wrote:
>
> Peter Eisentraut  writes:
> > On 21.04.23 16:28, Imseih (AWS), Sami wrote:
> >> I suggest a small doc fix:
> >> “Note that for a complex query, several sort or hash operations might be
> >> running simultaneously;”
>
> > Here is a discussion of these terms:
> > https://takuti.me/note/parallel-vs-concurrent/
>
> > I think "concurrently" is the correct word here.
>
> Probably, but it'd do little to remove the confusion Sami is on about,

+1.

When discussing this internally, Sami's proposal was in fact to use
the word 'concurrently'. But given that when it comes to computers and
programming, it's common for someone to not understand the intricate
difference between the two terms, we thought it's best to not use any
of those, and instead use a word not usually associated with
programming and algorithms.

Aside: Another pair of words I see regularly used interchangeably,
when in fact they mean different things: precise vs. accurate.

> especially since the next sentence uses "concurrently" to describe the
> other case.  I think we need a more thorough rewording, perhaps like
>
> -   Note that for a complex query, several sort or hash operations might 
> be
> -   running in parallel; each operation will generally be allowed
> +   Note that a complex query may include several sort or hash
> +   operations; each such operation will generally be allowed

This wording doesn't seem to bring out the fact that there could be
more than one work_mem consumer running (in-progress) at the same
time. The reader to could mistake it to mean hashes and sorts in a
complex query may happen one after the other.

+ Note that a complex query may include several sort and hash operations, and
+ more than one of these operations may be in progress simultaneously at any
+ given time;  each such operation will generally be allowed

I believe the phrase "several sort _and_ hash" better describes the
possible composition of a complex query, than does "several sort _or_
hash".

> I also find this wording a bit further down to be poor:
>
> Hash-based operations are generally more sensitive to memory
> availability than equivalent sort-based operations.  The
> memory available for hash tables is computed by multiplying
> work_mem by
> hash_mem_multiplier.  This makes it
>
> I think "available" is not le mot juste, and it's also unclear from
> this whether we're speaking of the per-hash-table limit or some
> (nonexistent) overall limit.  How about
>
> -   memory available for hash tables is computed by multiplying
> +   memory limit for a hash table is computed by multiplying

+1

Best regards,
Gurjeet https://Gurje.et
Postgres Contributors Team, http://aws.amazon.com

Mark a transaction uncommittable

2023-04-22 Thread Gurjeet Singh

This is a proposal for a new transaction characteristic. I haven't
written any code, yet, and am interested in hearing if others may find
this feature useful.

Many a times we start a transaction that we never intend to commit;
for example, for testing, or for EXPLAIN ANALYZE, or after detecting
unexpected results but still interested in executing more commands
without risking commit,  etc.

A user would like to declare their intent to eventually abort the
transaction as soon as possible, so that the transaction does not
accidentally get committed.

This feature would allow the user to mark a transaction such that it
can never be committed. We must allow such marker to be placed when
the transaction is being started, or while it's in progress.

Once marked uncommittable, do not allow the marker to be removed.
Hence, once deemed uncommittable, the transaction cannot be committed,
even intentionally. This protects against cases where one script
includes another (e.g. psql's \i command), and the included script may
have statements that turn this marker back on.

Any command that ends a transaction (END, COMMIT, ROLLBACK) must
result in a rollback.

All of these properties seem useful for savepoints, too. But I want to
focus on just the top-level transactions, first.

I feel like the BEGIN and SET TRANSACTION commands would be the right
places to introduce this feature.

BEGIN [ work | transaction ] [ [ NOT ] COMMITTABLE ];
SET TRANSACTION [ [ NOT ] COMMITTABLE ];

I'm not yet sure if the COMMIT AND CHAIN command should carry this
characteristic to the next transaction.

Thoughts?

Best regards,
Gurjeet https://Gurje.et
http://aws.amazon.com

Re: [PATCH] Infinite loop while acquiring new TOAST Oid

2023-04-22 Thread Gurjeet Singh

On Thu, Dec 22, 2022 at 10:07 AM Nikita Malakhov  wrote:
> Any suggestions on the previous message (64-bit toast value ID with 
> individual counters)?

Was this patch ever added to CommitFest? I don't see it in the current
Open Commitfest.

https://commitfest.postgresql.org/43/

Best regards,
Gurjeet http://Gurje.et
http://aws.amazon.com

Re: Doc limitation update proposal: include out-of-line OID usage per TOAST-ed columns

2023-04-22 Thread Gurjeet Singh

On Fri, Apr 21, 2023 at 12:14 AM Nikita Malakhov  wrote:
> This limitation applies not only to wide tables - it also applies to tables 
> where TOASTed values
> are updated very often. You would soon be out of available TOAST value ID 
> because in case of
> high frequency updates autovacuum cleanup rate won't keep up with the 
> updates. It is discussed
> in [1].
>
> On Fri, Apr 21, 2023 at 9:39 AM Jakub Wartak  
> wrote:
>> I would like to ask if it wouldn't be good idea to copy the
>> https://wiki.postgresql.org/wiki/TOAST#Total_table_size_limit
>> discussion (out-of-line OID usage per TOAST-ed columns / potential
>> limitation) to the official "Appendix K. PostgreSQL Limits" with also
>> little bonus mentioning the "still searching for an unused OID in
>> relation" notice. Although it is pretty obvious information for some
>> and from commit 7fbcee1b2d5f1012c67942126881bd492e95077e and the
>> discussion in [1], I wonder if the information shouldn't be a little
>> more well known via the limitation (especially to steer people away
>> from designing very wide non-partitioned tables).
>>
>> [1] - 
>> https://www.postgresql.org/message-id/flat/16722-93043fb459a41073%40postgresql.org
>
> [1] 
> https://www.postgresql.org/message-id/CAN-LCVPRvRzxeUdYdDCZ7UwZQs1NmZpqBUCd%3D%2BRdMPFTyt-bRQ%40mail.gmail.com

These 2 discussions show that it's a painful experience to run into
this problem, and that the hackers have ideas on how to fix it, but
those fixes haven't materialized for years. So I would say that, yes,
this info belongs in the hard-limits section, because who knows how
long it'll take this to be fixed.

Please submit a patch.

I anticipate that edits to Appendix K Postgres Limits will prompt
improving the note in there about the maximum column limit, That note
is too wordy, and sometimes confusing, especially for the audience
that it's written for: newcomers to Postgres ecosystem.

Best regards,
Gurjeet https://Gurje.et
http://aws.amazon.com

Re: Minor code de-duplication in fe-connect.c

2023-04-24 Thread Gurjeet Singh

On Mon, Apr 24, 2023 at 5:14 AM Daniel Gustafsson  wrote:

> > On 21 Apr 2023, at 18:38, Gurjeet Singh  wrote:
> >
> > On Fri, Apr 21, 2023 at 7:47 AM Robert Haas 
> wrote:
> >>
> >> On Fri, Apr 21, 2023 at 8:25 AM Daniel Gustafsson 
> wrote:
> >>> The reason I left it like this when reviewing and committing is that I
> think it
> >>> makes for more readable code.  The amount of lines saved is pretty
> small, and
> >>> "shuffle" isn't an exact term so by reading the code it isn't
> immediate clear
> >>> what such a function would do.  By having the shuffle algorithm where
> it's used
> >>> it's clear what the code does and what the outcome is.  If others
> disagree I
> >>> can go ahead and refactor of course, but I personally would not deem
> it a net
> >>> win in code quality.
> >>
> >> I think we should avoid nitpicking stuff like this. I likely would
> >> have used a subroutine if I'd done it myself, but I definitely
> >> wouldn't have submitted a patch to change whatever the last person did
> >> without some tangible reason for so doing.
> >
> > Code duplication, and the possibility that a change in one location
> > (bugfix or otherwise) does not get applied to the other locations, is
> > a good enough reason to submit a patch, IMHO.
> >
> >> It's not a good use of
> >> reviewer and committer time to litigate things like this.
> >
> > Postgres has a very high bar for code quality, and for documentation.
> > It is a major reason that attracts people to, and keeps them in the
> > Postgres ecosystem, as users and contributors. If anything, we should
> > encourage folks to point out such inconsistencies in code and docs
> > that keep the quality high.
> >
> > This is not a attack on any one commit, or author/committer of the
> > commit; sorry if it appeared that way. I was merely reviewing the
> > commit that introduced a nice libpq feature. This patch is merely a
> > minor improvement to the code that I think deserves a consideration.
> > It's not a litigation, by any means.
>
> I didn't actually read the patch earlier, but since Robert gave a +1 to the
> idea of refactoring I had a look. I have a few comments:
>
> +static void
> +shuffleAddresses(PGConn *conn)
> This fails to compile since the struct is typedefed PGconn.
>
>
> -   /*
> -* This is the "inside-out" variant of the Fisher-Yates shuffle
> -* algorithm. Notionally, we append each new value to the array
> -* and then swap it with a randomly-chosen array element (possibly
> -* including itself, else we fail to generate permutations with
> -* the last integer last).  The swap step can be optimized by
> -* combining it with the insertion.
> -*
>  * We don't need to initialize conn->prng_state here, because that
>  * already happened in connectOptions2.
>  */
> This also fails to compile since it removes the starting /* marker of the
> comment resulting in a comment starting on *.
>
>
> -   for (i = 1; i < conn->nconnhost; i++)
> -   for (int i = 1; i < conn->naddr; i++)
> You are replacing these loops for shuffling an array with a function that
> does
> this:
> +   for (int i = 1; i < conn->naddr; i++)
> This is not the same thing, one is shuffling the hosts and the other is
> shuffling the addresses resolved for the hosts (which may be a n:m
> relationship).  If you had compiled and run the tests you would have seen
> that
> the libpq tests now fail with this applied, as would be expected.
>
>
> Shuffling the arrays can of course be made into a subroutine, but what to
> shuffle and where needs to be passed in to it and at that point I doubt the
> code readability is much improved over the current coding.
>
>
Sorry about the errors. This seems to be the older version of the patch
that I had generated before fixing these mistakes. I do remember
encountering the compiler errors and revising the code to fix these,
especially the upper vs. lower case and the partial removal of the coment.
Away from my keyboard, so please expect the newer patch some time later.

Best regards,
Gurjeet
http://Gurje.et

Postgres Index Advisor status and GSoC (was: Re: Returning to Postgres community work)

2023-05-11 Thread Gurjeet Singh

On Tue, Aug 31, 2021 at 10:02 AM Gurjeet Singh  wrote:
> On Tue, Aug 31, 2021 at 8:04 AM Alvaro Herrera  
> wrote:
>
> > You know what I've heard?  That your index advisor module languishes
> > unmaintained and that there's no shortage of people wishing to use it.
>
> Now there's a masterclass in making someone feel great and guilty in
> the same sentence ;-)
>
> > Heck, we spent a lot of back-and-forth in the spanish mailing list
> > with somebody building a super obsolete version of Postgres just so that
> > they could compile your index advisor.  I dunno, if you have some spare
> > time, maybe updating that one would be a valuable contribution from
> > users' perspective.
>
> Aye-aye Capn' :-)

As part of helping the GSoC contributor getting onboard (see below), I
went through a similar process and had to figure out the Postgres
version, system packages, etc. (all ancient) that were needed to build
and use it. It's no fun having to deal with software from over a
decade ago :-(

> EDB folks reached out to me a few months ago to assign a license to
> the project, which I did and it is now a Postgres-licensed project
> [1].
>
> Given the above, it is safe to assume that this tool is at least being
> maintained by EDB, at least internally for their customers. I would
> request them to contribute the changes back in the open.

After over a year of conversations and follow-ups, a couple of months
ago EnterpriseDB finally made it clear that they won't be contributing
their changes back to the open-source version of Index Advisor. With
that avenue now closed, we can now freely pursue

> Regardless of that, please rest assured that I will work on making it
> compatible with the current supported versions of Postgres. Lack of
> time is not an excuse anymore :-)

Oh, how wrong was I :-)

I have a few updates on the current state and plans around the Index
Adviser extension.

I proposed Index Adviser as a potential project for GSoC 2023 [1].
Ahmed (CCd) has signed up as the contributor. The project has now been
accepted/funded by GSoC. The primary goal of the project is to support
all the active versions of Postgres. The extended goal is to support
additional index types. The extension currently supports Postgres
version 8.3, and BTree index type.

[1]: 
https://wiki.postgresql.org/wiki/GSoC_2023#pg_adviser_.2F_index_adviser:_Recommend_Potentially_Useful_Indexes

Best regards,
Gurjeet
http://Gurje.et

Re: Remove extraneous break condition in logical slot advance function

2023-10-21 Thread Gurjeet Singh

On Fri, Oct 20, 2023 at 7:30 PM Bharath Rupireddy
 wrote:
>
> Hi,
>
> There exists an extraneous break condition in
> pg_logical_replication_slot_advance(). When the end of WAL or moveto
> LSN is reached, the main while condition helps to exit the loop, so no
> separate break condition is needed. Attached patch removes it.
>
> Thoughts?

+1 for the patch.

The only advantage I see of the code as it stands right now is that it
avoids one last call to CHECK_FOR_INTERRUPTS() by break'ing early. I
don't think we'd lose much in terms of performance by making one (very
cheap, in common case) extra call of this macro.

Best regards,
Gurjeet
http://Gurje.et

Replace references to malloc() in libpq documentation with generic language

2023-10-23 Thread Gurjeet Singh

The commit message in the attached patch provides the reasoning, as follows:

The user does not benefit from knowing that libpq allocates some/all memory
using malloc(). Mentioning malloc() here has a few downsides, and almost no
benefits.

All the relevant mentions of malloc() are followed by an explicit
instruction to use PQfreemem() to free the resulting objects. So the
docs perform the sufficient job of educating the user on how to properly
free the memory.  But these mentions of malloc() may still lead an
inexperienced or careless user to (wrongly) believe that they may use
free() to free the resulting memory. They will be in a lot of pain until
they learn that (when linked statically) libpq's malloc()/free() cannot
be mixed with malloc()/free() of whichever malloc() library the client
application is being linked with.

Another downside of explicitly mentioning that objects returned by libpq
functions are allocated with malloc(), is that it exposes the implementation
details of libpq to the users. Such details are best left unmentioned so that
these can be freely changed in the future without having to worry about its
effect on client applications.

Whenever necessary, it is sufficient to tell the user that the objects/memory
returned by libpq functions is allocated on the heap. That is just enough
detail for the user to realize that the relevant object/memory needs to be
freed; and the instructions that follow mention to use PQfreemem() to free such
memory.

One mention of malloc is left intact, because that mention is unrelated to how
the memory is allocated, or how to free it.

In passing, slightly improve the language of PQencryptPasswordConn()
documentation.

Best regards,
Gurjeet
http://Gurje.et


v1-0001-Replace-references-to-malloc-in-libpq-documentati.patch
Description: Binary data

Re: simplehash: preserve consistency in case of OOM

2023-11-17 Thread Gurjeet Singh

On Fri, Nov 17, 2023 at 12:13 PM Andres Freund  wrote:
>
> On 2023-11-17 10:42:54 -0800, Jeff Davis wrote:
> > Right now, if allocation fails while growing a hashtable, it's left in
> > an inconsistent state and can't be used again.

+1 to the patch.

> I'm not against allowing this - but I am curious, in which use cases is this
> useful?

I don't have an answer to that, but a guess would be when the server
is dealing with memory pressure. In my view the patch has merit purely
on the grounds of increasing robustness.

> > @@ -446,10 +459,11 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void 
> > *private_data)
> >   /* increase nelements by fillfactor, want to store nelements elements 
> > */
> >   size = Min((double) SH_MAX_SIZE, ((double) nelements) / 
> > SH_FILLFACTOR);
> >
> > - SH_COMPUTE_PARAMETERS(tb, size);
> > + size = SH_COMPUTE_SIZE(size);
> >
> > - tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, 
> > sizeof(SH_ELEMENT_TYPE) * tb->size);
> > + tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, 
> > sizeof(SH_ELEMENT_TYPE) * size);
> >
> > + SH_UPDATE_PARAMETERS(tb, size);
> >   return tb;
> >  }
>
> Maybe add a comment explaining why it's important to update parameters after
> allocating?

Perhaps something like this:

+ /*
+ * Update parameters _after_ allocation succeeds; prevent
+ * bogus/corrupted state.
+ */
+ SH_UPDATE_PARAMETERS(tb, size);

Best regards,
Gurjeet
http://Gurje.et

Re: Change GUC hashtable to use simplehash?

2023-11-17 Thread Gurjeet Singh

On Fri, Nov 17, 2023 at 11:02 AM Jeff Davis  wrote:
>
> I had briefly experimented changing the hash table in guc.c to use
> simplehash. It didn't offer any measurable speedup, but the API is
> slightly nicer.
>
> I thought I'd post the patch in case others thought this was a good
> direction or nice cleanup.

This is not a comment on the patch itself, but since GUC operations
are not typically considered performance or space sensitive, this
comment from simplehash.h makes a case against it.

 *  It's probably not worthwhile to generate such a specialized
implementation
 *  for hash tables that aren't performance or space sensitive.

But your argument of a nicer API might make a case for the patch. I

Best regards,
Gurjeet
http://Gurje.et

Re: Document that server will start even if it's unable to open some TCP/IP ports

2023-09-12 Thread Gurjeet Singh

On Fri, Sep 8, 2023 at 7:52 AM Bruce Momjian  wrote:
>
> On Thu, Sep  7, 2023 at 09:21:07PM -0700, Nathan Bossart wrote:
> > On Thu, Sep 07, 2023 at 07:13:44PM -0400, Bruce Momjian wrote:
> > > On Thu, Sep  7, 2023 at 02:54:13PM -0700, Nathan Bossart wrote:
> > >> IMO the phrase "open a port" is kind of nonstandard.  I think we should 
> > >> say
> > >> something along the lines of
> > >>
> > >>If listen_addresses is not empty, the server will start only if it can
> > >>listen on at least one of the specified addresses.  A warning will be
> > >>emitted for any addresses that the server cannot listen on.
> > >
> > > Good idea, updated patch attached.
> >
> > I still think we should say "listen on an address" instead of "open a
> > port," but otherwise it LGTM.
>
> Agreed, I never liked the "port" mention.  I couldn't figure how to get
> "open" out of the warning sentence though.  Updated patch attached.

LGTM.

Best regards,
Gurjeet
http://Gurje.et

Re: Document that server will start even if it's unable to open some TCP/IP ports

2023-10-01 Thread Gurjeet Singh

On Tue, Sep 26, 2023 at 4:02 PM Bruce Momjian  wrote:
>
> Patch applied back to PG 11.

+Peter E. since I received the following automated note:

> Closed in commitfest 2023-09 with status: Moved to next CF (petere)

Just a note that this patch has been committed (3fea854691), so I have
marked the CF item [1] as 'Committed', and specified Bruce as the
committer.

[1]: https://commitfest.postgresql.org/45/4333/

Best regards,
Gurjeet
http://Gurje.et

Re: [PoC/RFC] Multiple passwords, interval expirations

2023-10-04 Thread Gurjeet Singh

u1 VALID UNTIL '2022/01/01';
-- Note that currently I don't have a proposal for how to change the old
-- password's validity period, without deleting the latest/main password. See
-- PASSWORD NULL command below on how to delete the current password. It's very
-- likely that in a password rollover use case it's unnecessary, even
-- undesirable, to change the old password's validity period.

-- If, for some reason, the user wants to get rid of the latest password added.
-- Remove 'p2' (current password). The old password (p1), will be restored to
-- rolpassword, along with its valid-until value.
ALTER ROLE u1 PASSWORD NULL;
-- This may come as a surprise to some users, because currently they expect the
-- user to completely lose the ability to use passwords for login after this
-- command. To get the old behavior, the user must now use the ALL PASSWORD
-- NULL incantation; see below.
-- Issuing this command one more time will remove even the restored password,
-- hence leaving the user with no passwords.

-- Change the validity of the restored/current password (p1)
ALTER ROLE u1 VALID UNTIL '2022/01/01';

-- Add a new password (p3) without affecting old password's (p1) validity
ALTER ROLE u1 ADD PASSWORD 'p3' VALID UNTIL '2023/01/01';

-- Add a new password 'p4'. This will move 'p3' to rololdpassword, and hence
-- 'p1' will be forgotten completely.
-- After this command, user can use passwords 'p3' (old) and 'p4' (current) to
-- login.
ALTER ROLE u1 ADD PASSWORD 'p4' VALID UNTIL '2024/01/01';

-- Replace 'p4' (current) password with 'p5'. Note that this command is _not_
-- using the ADD keyword, hence 'p4' is _not_ moved to rololdpassword column.
-- After this command, user can use passwords 'p3' (old) and 'p5'
-- (current) to login.
ALTER ROLE u1 PASSWORD 'p5' VALID UNTIL '2025/01/01';

-- Using the old password to login will produce a warning, hopefully nudging
-- the user to start using the new password.
export PGPASSWORD=p3
psql -U u1
...
WARNING: Used old password to login

export PGPASSWORD=p5
psql -U u1
...
=> (no warning)

-- Remove all passwords from the role. Even old password, if any, is removed.
ALTER ROLE u1 ALL PASSWORD NULL;

In normal use, the users can simply keep ADDing new passwords, and the
database will promptly remember only one old password, and keep
forgetting any passwords older than that. But on the off chance
that someone needs to forget the latest password they added, and
restore the old password to be the "current" password, they can use
the PASSWORD NULL incantation. Note that this will result in
rol*old*password being set to NULL, because our password memory stack
cannot hold more than 2 elements.

Since this feature is targeted towards password rollovers, it's a legitimate
question to ask if we should enforce that the new password being added has a
valid-until greater than the valid-until of the existing/old password. I don't
think we should enforce this, at least not in this patch, because the
user/admin may have a use case where they want a short-lived new password that
they intend/want to change very soon; I'm thinking of cases where passwords are
being rolled over while they are also moving from older clients/libraries that
don't yet support scram-sha-256; keep using md5 and add passwords to honor
password rollover policy, but then as soon as all clients have been updated and
have the ability to use scram-sha-256, rollover the password again to utilize
the better mechanism.

I realize that allowing for a maximum of 2 passwords goes against the
zero-one-infinity rule [1], but I think in the case of password
rollovers it's perfectly acceptable to limit the number of active
passwords to just 2. If there are use cases, either related to password
rollovers, or in its vicinity, that can be better addressed by
allowing an arbitrarily many passwords, I would love to learn about
them and change this design to accommodate for those use cases, or
perhaps revert to pursuing the multiple-passwords feature.

[1]: https://en.wikipedia.org/wiki/Zero_one_infinity_rule

Best regards,
Gurjeet
http://Gurje.et

Re: Good News Everyone! + feature proposal

2023-10-05 Thread Gurjeet Singh

On Thu, Oct 5, 2023 at 1:52 AM Jon Erdman  wrote:
>
> So I have some good news! At long last I've found a company/manager that
> wants to actually factually pay me to do some work on PG!!

Congratulations!

> For the proposal (this one is a bit Apple specific): because my team
> offers managed postgres to our Apple-internal customers, many of whom
> are not database experts, or at least not postgres experts, we'd like to
> be able to toggle the availability of UNLOGGED tables in
> postgresql.conf, so our less clueful users have fewer footguns.
>
> So, my proposal is for a GUC to implement that, which would *OF COURSE*
> undefault to allowing UNLOGGED.

That was difficult to parse at first glance. I guess you mean the
GUC's default value will not change the current behaviour, as you
mention below.

> The reasoning here is we have autofailover set up for our standard
> cluster offering that we give to customers, using sync-rep to guarantee
> no data loss if we flop to the HA node. Any users not up to speed on
> what UNLOGGED really means could inadvertently lose data, so we'd like
> to be able to force it to be off, and turn it on upon request after
> educating the customer in question it's it's a valid use case.
>
> So to begin with: I'm sure some folks will hate this idea, but maybe a
> good many wont, and remember, this would default to UNLOGGED enabled, so
> no change to current behaviour. and no encouragement to disallow it, but
> just the ability to do so, which i think is useful in
> hosted/managed/multi-tenant environment where most things are taken care
> of for the customer.

I see the need to disable this feature and agree that some
installations may need it, where the users are not savvy enough to
realize its dangers and fall for its promise to increase INSERT/UPDATE
performance. Your specific example of an internal hosted/managed
service is a good example. Even in smaller installations the DBA might
want to disable this, so that unwary developers don't willy-nilly
create unlogged tables and end up losing data after a failover. I hope
others can provide more examples and situations where this may be
useful, to make it obvious that we need this feature.

My first reaction was to make it a GRANTable permission. But since one
can always do `ALTER USER savvy_user SET allow_unlogged_tables TO
true` and override the system-wide setting in postgresql.conf, for a
specific user, I feel a GUC would be the right way to implement it.

The complaint about too many GUCs is a valid one, but I'd worry more
about it if it were an optimizer/performance improvement being hidden
behind a GUC. This new GUC would be a on-off switch to override the
SQL/grammar feature provided by the UNLOGGED keyword, hence not really
a concern IMO.

> So I'd like to get a general idea how likely this would be to getting
> accepted if it did it, and did it right?

Like others said, there are no guarantees. A working patch may help
guide people's opinion one way or the other, so I'd work on submitting
a patch while (some) people are in agreement.

> Let the flame war begin!

Heh. I'm sure you already know this, but this community's flame wars
are way more timid compared to what members of other communities may
be used to :-) I consider it lucky if someone throws as much as a lit
match.

> PS: I'm SO happy that this phase of my postgres journey has finally
> started

I, too, am very happy for you! :-)

> Jon Erdman (aka StuckMojo on IRC)

TIL.

I wish there was a directory of IRC identities that pointed to real
identities (at least for folks who don't mind this mapping available
in the open), so that when we converse in IRC, we have a face to go
with the IRC handles. As a human I feel that necessity.

Best regards,
Gurjeet
http://Gurje.et

Re: [PoC/RFC] Multiple passwords, interval expirations

2023-10-05 Thread Gurjeet Singh

On Thu, Oct 5, 2023 at 12:04 PM Nathan Bossart  wrote:
>
> On Wed, Oct 04, 2023 at 10:41:15PM -0700, Gurjeet Singh wrote:
> > The patches posted in this thread so far attempt to add the ability to
> > allow the user to have an arbitrary number of passwords. I believe
> > that allowing arbitrary number of passwords is not only unnecessary,
> > but the need to name passwords, the need to store them in a shared
> > catalog, etc. may actually create problems in the field. The
> > users/admins will have to choose names for passwords, which they
> > didn't have to previously. The need to name them may also lead to
> > users storing password-hints in the password names (e.g. 'mom''s
> > birthday', 'ex''s phone number', 'third password'), rendering the
> > passwords weak.
> >
> > Moreover, allowing an arbitrarily many number of passwords will
> > require us to provide additional infrastructure to solve problems like
> > observability (which passwords are currently in use, and which ones
> > have been effectively forgotten by applications), or create a nuisance
> > for admins that can create more problems than it solves.
>
> IMHO neither of these problems seems insurmountable.

Agreed.

> Besides advising
> against using hints as names, we could also automatically generate safe
> names, or even disallow user-provided names entirely.

Somehow naming passwords doesn't feel palatable to me.

> And adding
> observability for passwords seems worthwhile anyway.

Agreed.

> > So I propose that the feature should allow no more than 2 passwords
> > for a role, each with their own validity periods. This eliminates the
> > need to name passwords, because at any given time there are no more
> > than 2 passwords; current one, and old one. This also eliminates the
> > need for a shared catalog to hold passwords, because with the limit of
> > 2 imposed, we can store the old password and its validity period in
> > additional columns in the pg_authid table.
>
> Another approach could be to allow an abritrary number of passwords but to
> also allow administrators to limit how many passwords can be associated to
> each role.  That way, we needn't restrict this feature to 2 passwords for
> everyone.  Perhaps 2 should be the default, but in any case, IMO we
> shouldn't design to only support 2.

I don't see a real use case to support more than 2 passwords. Allowing
an arbitrary number of passwords might look good and clean from
aesthetics and documentation perspective (no artificially enforced
limits, as in the zero-one-infinity rule), but in absence of real use
cases for that many passwords, I'm afraid we might end up with a
feature that creates more and worse problems for the users than it
solves.

> > In essence, we create a stack that can hold 2 passwords. Pushing an
> > element when it's full will make it forget the bottom element. Popping
> > the stack makes it forget the top element, and the only remaining
> > element, if any, becomes the top.
>
> I think this would be mighty confusing to users since it's not clear that
> adding a password will potentially invalidate a current password (which
> might be actively in use), but only if there are already 2 in the stack.

Fair point. We can aid the user by emitting a NOTICE (or a WARNING)
message whenever an old password is removed from the system because of
addition of a new password.

> I
> worry that such a desіgn might be too closely tailored to the
> implementation details.  If we proceed with this design, perhaps we should
> consider ERROR-ing if a user tries to add a third password.

I did not have a stack in mind when developing the use case and the
grammar, so implementation details did not drive this design. This new
design was more of a response to the manageability nightmare that I
could see the old approach may lead to. When writing the email I
thought mentioning the stack analogy may make it easier to develop a
mental model. I certainly won't suggest using it in the docs for
explanation :-)

> > -- If, for some reason, the user wants to get rid of the latest password 
> > added.
> > -- Remove 'p2' (current password). The old password (p1), will be restored 
> > to
> > -- rolpassword, along with its valid-until value.
> > ALTER ROLE u1 PASSWORD NULL;
> > -- This may come as a surprise to some users, because currently they expect 
> > the
> > -- user to completely lose the ability to use passwords for login after this
> > -- command. To get the old behavior, the user must now use the ALL PASSWORD
> > -- NULL incantation; see below.
> > -- Issuing this command one more

Re: [PoC/RFC] Multiple passwords, interval expirations

2023-10-05 Thread Gurjeet Singh

On Thu, Oct 5, 2023 at 1:09 PM Jeff Davis  wrote:
>
> >
> > I think this would be mighty confusing to users since it's not clear
> > that
> > adding a password will potentially invalidate a current password
> > (which
> > might be actively in use), but only if there are already 2 in the
> > stack.  I
> > worry that such a desіgn might be too closely tailored to the
> > implementation details.  If we proceed with this design, perhaps we
> > should
> > consider ERROR-ing if a user tries to add a third password.
>
> I agree that the proposed language is confusing, especially because ADD
> causes a password to be added and another one to be removed. But
> perhaps there are some non-confusing ways to expose a similar idea.

How about a language like the following (I haven't tried if this will
work in the grammar we have):

CREATE ROLE u1 PASSWORD 'p1';

ALTER ROLE u1ADD NEW PASSWORD 'p2';

ALTER ROLE u1 ADD NEW PASSOWRD 'p3';
ERROR: Cannot have more than 2 passwords at the same time.

ALTER ROLE u1 DROP OLD PASSWORD;

ALTER ROLE u1 ADD NEW PASSOWRD 'p3';
-- succeeds; forgets password 'p1'; p2 and p3 can be used to login

ALTER ROLE u1 DROP NEW PASSWORD;
-- forgets password 'p3'. Only 'p2' can be used to login

ALTER ROLE u1 ADD NEW PASSOWRD 'p4';
-- succeeds; 'p2' and 'p4' can be used to login

-- Set the valid-until of 'new' (p4) password
ALTER ROLE u1 VALID UNTIL '2024/01/01';

-- If we need the ability to change valid-until of both old and new,
we may allow something like the following.
ALTER ROLE u1 [_NEW_ | OLD] VALID UNTIL '2024/01/01';

This way there's a notion of a 'new' and 'old' passwords. User cannot
add a third password without explicitly dropping one of existing
passwords (either old or new). At any time the user can choose to drop
the old or the new password. Adding a new password will mark the
current password as 'old'; if there's only old password (because 'new'
was dropped) the 'old' password will remain intact and the new one
will be placed in 'current'/new spot.

So in normal course of operation, even for automated jobs, the
expected flow to roll over the passwords would be:

ALTER USER u1 DROP OLD PASSWORD;
-- success if there is an old password; otherwise NOTICE: no old password
ALTER USER u1 ADD NEW PASSWORD 'new-secret';

> The thing I like about Gurjeet's proposal is that it's well-targeted at
> a specific use case rather than trying to be too general. That makes it
> a little easier to avoid certain problems like having a process that
> adds passwords and never removes the old ones (leading to weird
> problems like 47000 passwords for one user).
>
> But it also feels strange to be limited to two -- perhaps the password
> rotation schedule or policy just doesn't work with a limit of two, or
> perhaps that introduces new kinds of mistakes.
>
> Another idea: what if we introduce the notion of deprecating a
> password?

I'll have to think more about it, but perhaps my above proposal
addresses the use case you describe.

> if we allow multiple deprecated passwords, we'd still have to come up
> with some way to address them (names, numbers, validity period,
> something). But by isolating the problem to deprecated passwords only,
> it feels like the system is still being restored to a clean state with
> at most one single current password. The awkwardness is contained to
> old passwords which will hopefully go away soon anyway and not
> represent permanent clutter.

+1

Best regards,
Gurjeet
http://Gurje.et

Re: [PoC/RFC] Multiple passwords, interval expirations

2023-10-08 Thread Gurjeet Singh

On Fri, Oct 6, 2023 at 1:46 PM Bruce Momjian  wrote:
>
> On Fri, Oct  6, 2023 at 01:20:03PM -0700, Jeff Davis wrote:
> > The basic problem, as I see it, is: how do we keep users from
> > accidentally dropping the wrong password? Generated unique names or
>
> I thought we could auto-remove old password if the valid-until date is
> in the past.

Autoremoving expired passwords will surprise users, and not in a good
way. Making a password, even an expired one, disappear from the system
will lead to astonishment. Among uses of an expired password are cases
of it acting like a tombstone, and the case where the user may want to
extend the validity of a password, instead of having to create a new
one and change application configuration(s) to specify the new
password.

Best regards,
Gurjeet
http://Gurje.et

Re: [PoC/RFC] Multiple passwords, interval expirations

2023-10-08 Thread Gurjeet Singh

On Sun, Oct 8, 2023 at 10:29 AM Bruce Momjian  wrote:
>
> I was speaking of autoremoving in cases where we are creating a new one,
> and taking the previous new one and making it the old one, if that was
> not clear.

Yes, I think I understood it differently. I understood it to mean that
this behaviour would apply to all passwords, those created by existing
commands, as well as to those created by new commands for rollover use
case. Whereas you meant this autoremove behaviour to apply only to
those passwords created by/for rollover related commands. I hope I've
understood your proposal correctly this time around :-)

I believe the passwords created by rollover feature should
behave by the same rules as the rules for passwords created by
existing CREATE/ALTER ROLE commands. If we implement the behaviour to
delete expired passwords, then I believe that behaviour should apply
to all passwords, irrespective of which command/feature was used to
create a password.

Best regards,
Gurjeet
http://Gurje.et

Re: [PoC/RFC] Multiple passwords, interval expirations

2023-10-08 Thread Gurjeet Singh

On Fri, Oct 6, 2023 at 1:29 PM Jeff Davis  wrote:
>
> On Thu, 2023-10-05 at 14:28 -0700, Gurjeet Singh wrote:
>
> > This way there's a notion of a 'new' and 'old' passwords.
>
> IIUC, you are proposing that there are exactly two slots, NEW and OLD.
> When adding a password, OLD must be unset and it moves NEW to OLD, and
> adds the new password in NEW. DROP only works on OLD. Is that right?

Yes, that's what I was proposing. But thinking a bit more about it,
the _implicit_ shuffling of labels 'new' and 'old' doesn't feel right
to me. The password that used to be referred to as 'new' now
automatically gets labeled 'old'.

> It's close to the idea of deprecation, except that adding a new
> password implicitly deprecates the existing one. I'm not sure about
> that -- it could be confusing.

+1

> We could also try using a verb like "expire" that could be coupled with
> a date, and that way all old passwords would always have some validity
> period.

Forcing the users to pick an expiry date for a password they intend to
rollover, when an expiry date did not exist before for that password,
feels like adding more burden to their password rollover decision
making. The dates and rules of password rollover may be a part of a
system external to their database, (wiki, docs, calendar, etc.) which
now they will be forced to translate into a timestamp to specify in
the rollover commands.

I believe we should fix the _names_ of the slots the 2 passwords are
stored in, and provide commands that manipulate those slots by
respective names; the commands should not implicitly move the
passwords between the slots. Additionally, we may provide functions
that provide observability info for these slots. I propose the slot
names FIRST and SECOND (I picked these because these keywords/tokens
already exist in the grammar, but not yet sure if the grammar rules
would allow their use; feel free to propose better names). FIRST
refers to the the existing slot, namely rolpassword. SECOND refers to
the new slot we'd add, that is, a pgauthid column named
rolsecondpassword. The existing commands, or when neither FIRST nor
SECOND are specified, the commands apply to the existing slot, a.k.a.
FIRST.

The user interface might look like the following:

-- Create a user, as usual
CREATE ROLE u1 PASSWORD 'p1' VALID UNTIL '2020/01/01';
-- This automatically occupies the 'first' slot

-- Add another password that the user can use for authentication.
ALTER ROLE u1 ADD SECOND PASSWORD 'p2' VALID UNTIL '2021/01/01';

-- Change both the passwords' validity independently; this solves the
-- problem with the previous '2-element stack' approach, where we
-- could not address the password at the bottom of the stack.
ALTER ROLE u1 SECOND PASSWORD VALID UNTIL '2022/01/01';

ALTER ROLE u1 [ [ FIRST ] PASSWORD ] VALID UNTIL '2022/01/01';

-- If, for some reason, the user wants to get rid of the latest password added.
ALTER ROLE u1 DROP SECOND PASSWORD;

-- Add a new password (p3) in 'second' slot
ALTER ROLE u1 ADD SECOND PASSWORD 'p3' VALID UNTIL '2023/01/01';

-- Attempting to add a password while the respective slot is occupied
-- results in error
ALTER ROLE u1 ADD [ [ FIRST ] PASSWORD ] 'p4' VALID UNTIL '2024/01/01';
ERROR: first password already exists

ALTER ROLE u1 ADD SECOND PASSWORD 'p4' VALID UNTIL '2024/01/01';
ERROR: second password already exists

-- Users can use this function to check whether a password slot is occupied
=> select password_exists('u1', 'first');
password_exists
-
t

=> select password_exists('u1', 'second');
password_exists
-
t

-- Remove all passwords from the role. Both, 'first' and 'second',
passwords are removed.
ALTER ROLE u1 DROP ALL PASSWORD;

Best regards,
Gurjeet
http://Gurje.et

Re: [PoC/RFC] Multiple passwords, interval expirations

2023-10-09 Thread Gurjeet Singh

On Sun, Oct 8, 2023 at 1:01 PM Gurjeet Singh  wrote:
>
> On Fri, Oct 6, 2023 at 1:29 PM Jeff Davis  wrote:
> >
> > On Thu, 2023-10-05 at 14:28 -0700, Gurjeet Singh wrote:
> >
> > > This way there's a notion of a 'new' and 'old' passwords.
> >
> > IIUC, you are proposing that there are exactly two slots, NEW and OLD.
> > When adding a password, OLD must be unset and it moves NEW to OLD, and
> > adds the new password in NEW. DROP only works on OLD. Is that right?
>
> Yes, that's what I was proposing. But thinking a bit more about it,
> the _implicit_ shuffling of labels 'new' and 'old' doesn't feel right
> to me. The password that used to be referred to as 'new' now
> automatically gets labeled 'old'.
>
> > It's close to the idea of deprecation, except that adding a new
> > password implicitly deprecates the existing one. I'm not sure about
> > that -- it could be confusing.
>
> +1

> I believe we should fix the _names_ of the slots the 2 passwords are
> stored in, and provide commands that manipulate those slots by
> respective names; the commands should not implicitly move the
> passwords between the slots. Additionally, we may provide functions
> that provide observability info for these slots. I propose the slot
> names FIRST and SECOND (I picked these because these keywords/tokens
> already exist in the grammar, but not yet sure if the grammar rules
> would allow their use; feel free to propose better names). FIRST
> refers to the the existing slot, namely rolpassword. SECOND refers to
> the new slot we'd add, that is, a pgauthid column named
> rolsecondpassword. The existing commands, or when neither FIRST nor
> SECOND are specified, the commands apply to the existing slot, a.k.a.
> FIRST.

Please see attached the patch that implements this proposal. The patch
is named password_rollover_v3.diff, breaking from the name
'multiple_passwords...', since this patch limits itself to address the
password-rollover feature.

The multiple_password* series of patches had removed a critical
functionality, which I believe is crucial from security perspective.
When a user does not exist, or has no passwords, or has passwords that
have expired, we must pretend to perform authentication (network
packet exchanges) as normally as possible, so that the absence of
user, or lack of (or expiration of) passwords is not revealed to an
attacker. I have restored the original behaviour in the
CheckPWChallengeAuth() function; see commit aba99df407 [2].

I looked for any existing keywords that may better fit the purpose of
naming the slots, better than FIRST and SECOND, but I could not find
any. Instead of DROP to remove the passwords, I tried DELETE and the
grammar/bison did not complain about it; so DELETE is an option, too,
but I feel DROP FIRST/SECOND/ALL PASSWORD is a better companion of ADD
FIRST/SECOND PASSWORD syntax, in the same vein as ADD/DROP COLUMN.

The doc changes are still missing, but the regression tests and the
comments therein should provide a good idea of the user interface of
this new feature. Documenting this behaviour in a succinct manner
feels difficult; so ideas welcome for how to inform the reader that
now a role is accompanied by two slots to store the passwords, and
that the old commands operate on the first slot, and to operate on the
second password slot one must use the new syntax. I guess it would be
best to start the relevant section with "To support gradual password
rollovers, Postgres provides the ability to store 2 active passwords
at the same time. The passwords are referred to as FIRST and SECOND
password. Each of these passwords can be changed independently, and
each of these can have an associated expiration time, if necessary."

Since these new commands are only available to ALTER ROLE (and not to
CREATE ROLE), the corresponding command doc page also needs to be
updated.

Next steps:
- Break the patch into a series of smaller patches.
- Add TAP tests (test the ability to actually login with these passwords)
- Add/update documentation
- Add more regression tests

The patch progress can be followed on the Git branch
password_rollover_v3 [1]. This branch begins uses
multiple_passwords_v4 as starting point, and removes unnecessary code
(commit 326f60225f [3])

The v1 (and tombstone of v2) patches of password_rollover never
finished as the consensus changed while they were in progress, but
they exist as sibling branches of the v3 branch.

[1]: https://github.com/gurjeet/postgres/commits/password_rollover_v3
[2]: 
https://github.com/gurjeet/postgres/commit/aba99df407a523357db2813f0eea0b45dbeb6006
[3]: 
https://github.com/gurjeet/postgres/commit/326f60225f0e660338fc9c276c8728dc10db435b

Best regards,
Gurjeet
http://Gurje.et

password_rollover_v3.diff
Description: Binary data

Re: [PoC/RFC] Multiple passwords, interval expirations

2023-10-09 Thread Gurjeet Singh

On Mon, Oct 9, 2023 at 2:31 AM Gurjeet Singh  wrote:
>
> Next steps:
> - Break the patch into a series of smaller patches.

Please see attached the same v3 patch, but now split into 3 separate
patches. Each patch in the series depends on the previous patch to
have been applied. I have made sure that each patch passes `make
check` individually.

First patch adds the two new columns, rolsecondpassword and
rolsecondvaliduntil to the pg_authid shared catalog. This patch also
updates the corresponding pg_authid.dat file to set these values to
null for the rows populated during bootstrap. Finally, it adds code to
CreateRole() to set these columns' values to NULL for a role being
created.

The second patch updates the password extraction, verification
functions as well as authentication functions to honor the second
password, if any. There is more detailed description in the commit
message/body of the patch.

The third patch adds the SQL support to the ALTER ROLE command which
allows manipulation of both, the rolpassword and rolsecondpassword,
columns and their respective expiration timestamps,
rol[second]validuntil. This patch also adds regression tests for the
new SQL command, demonstrating the use of the new grammar.

v3-0001-Add-new-columns-to-pg_authid.patch
v3-0002-Update-password-verification-infrastructure-to-ha.patch
v3-0003-Added-SQL-support-for-ALTER-ROLE-to-manage-two-pa.patch



Best regards,
Gurjeet
http://Gurje.et
From bc7c35e53e421157c9425c198bc2557ad118a650 Mon Sep 17 00:00:00 2001
From: Gurjeet Singh 
Date: Mon, 9 Oct 2023 11:36:05 -0700
Subject: [PATCH v3 1/3] Add new columns to pg_authid

Add two columns to pg_authid, namely rolsecondpassword and
rolsecondvaliduntil. These columns are added in preparation for the
password-rollover feature. These columns will store the hash and the
expiration time, repspectively, of a second password that the role can
use for login authentication.
---
 src/backend/commands/user.c   |  4 +++
 src/include/catalog/pg_authid.dat | 45 ---
 src/include/catalog/pg_authid.h   |  2 ++
 3 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index ce77a055e5..0afaf74865 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -452,9 +452,13 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 	else
 		new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
 
+	new_record_nulls[Anum_pg_authid_rolsecondpassword - 1] = true;
+
 	new_record[Anum_pg_authid_rolvaliduntil - 1] = validUntil_datum;
 	new_record_nulls[Anum_pg_authid_rolvaliduntil - 1] = validUntil_null;
 
+	new_record_nulls[Anum_pg_authid_rolsecondvaliduntil - 1] = true;
+
 	new_record[Anum_pg_authid_rolbypassrls - 1] = BoolGetDatum(bypassrls);
 
 	/*
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 6b4a0d..b326e48376 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -23,76 +23,91 @@
   rolname => 'POSTGRES', rolsuper => 't', rolinherit => 't',
   rolcreaterole => 't', rolcreatedb => 't', rolcanlogin => 't',
   rolreplication => 't', rolbypassrls => 't', rolconnlimit => '-1',
-  rolpassword => '_null_', rolvaliduntil => '_null_' },
+  rolpassword => '_null_', rolvaliduntil => '_null_',
+  rolsecondpassword => '_null_', rolsecondvaliduntil => '_null_' },
 { oid => '6171', oid_symbol => 'ROLE_PG_DATABASE_OWNER',
   rolname => 'pg_database_owner', rolsuper => 'f', rolinherit => 't',
   rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
   rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
-  rolpassword => '_null_', rolvaliduntil => '_null_' },
+  rolpassword => '_null_', rolvaliduntil => '_null_',
+  rolsecondpassword => '_null_', rolsecondvaliduntil => '_null_' },
 { oid => '6181', oid_symbol => 'ROLE_PG_READ_ALL_DATA',
   rolname => 'pg_read_all_data', rolsuper => 'f', rolinherit => 't',
   rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
   rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
-  rolpassword => '_null_', rolvaliduntil => '_null_' },
+  rolpassword => '_null_', rolvaliduntil => '_null_',
+  rolsecondpassword => '_null_', rolsecondvaliduntil => '_null_' },
 { oid => '6182', oid_symb

Minor edit to src/bin/pg_upgrade/IMPLEMENTAION

2023-10-09 Thread Gurjeet Singh

The attached patch adds the word 'databases' to show that template0,
template1 and postgres are databases in a default installation.

Best regards,
Gurjeet
http://Gurje.et


pg_upgrade.implementation.diff
Description: Binary data

Re: pg_reload_conf() synchronously

2022-11-05 Thread Gurjeet Singh

On Sat, Nov 5, 2022 at 11:23 AM Andres Freund  wrote:
> > As far as I know, Gurjeet has been annoyed only with non-user-settable
> > GUCs for one connection (correct me of course), there was nothing
> > fancy with isolation tests, yet.  Not saying that this is useless for
> > isolation tests, this would have its cases for assumptions where
> > multiple GUCs need to be synced across multiple sessions, but it seems
> > to me that we have two different cases in need of two different
> > solutions.
>
> I didn't say we need to go for something more complete. Just that it's worth
> thinking about.

FWIW, I have considered a few different approaches, but all of them
were not only more work, they were fragile, and diffcult to prove
correctness of. For example, I thought of implementing DSM based
synchronisation between processes, so that the invoking backend can be
sure that _all_ children of Postmaster have processed the reload. But
that will run into problems as different backends get created, and as
they exit.

The invoking client may be interested in just client-facing backends
honoring the reload before moving on, so it would have to wait until
all the other backends finish their current command and return to the
main loop; but that may never happen, because one of the backends may
be processing a long-running query. Or, for some reason, the the
caller may be interested in only the autovacuum proccesses honoring
its reload request. So I thought about creating _classes_ of backends:
client-facing, other always-on children of Postmaster, BGWorkers, etc.
But that just makes the problem more difficult to solve.

I hadn't considered the possibilty of deadlocks that Tom raised, so
that's another downside of the other approaches.

The proposed patch solves a specific problem, that of a single backend
reloading conf before the next command, without adding any complexity
for any other cases. It sure is worth solving the case where a backend
waits for another backed(s) to process the conf files, but it will
have to be a separate solution, becuase it's much more difficult to
get it right.

Best regards,
Gurjeet
http://Gurje.et

Re: Named Operators

2023-01-20 Thread Gurjeet Singh

On Sat, Jan 14, 2023 at 6:14 AM Gurjeet Singh  wrote:
>
> I agree that an identifier _surrounded_ by the same token (e.g. #foo#)
> or the pairing token (e.g. {foo}) looks better aesthetically, so I am
> okay with any of the following variations of the scheme, as well:
>
> \#foo\#  (tested; works)
> \#foo#   (not tested; reduces ident length by 1)
>
> We can choose a different character, instead of #. Perhaps \{foo} !

Please find attached the patch that uses \{foo} styled Named
Operators. This is in line with Tom's reluctant hint at possibly using
curly braces as delimiter characters. Since the curly braces are used
by the SQL Specification for row pattern recognition, this patch
proposes escaping the first of the curly braces.

We can get rid of the leading backslash, if (a) we're confident that
SQL committee will not use curly braces anywhere else, and (b) if
we're confident that if/when Postgres supports Row Pattern Recognition
feature, we'll be able to treat curly braces inside the PATTERN clause
specially. Since both of those conditions are unlikely, I think we
must settle for the escaped-first-curly-brace style for the naming our
operators.

Keeping with the previous posts, here's a sample SQL script showing
what the proposed syntax will look like in action. Personally, I
prefer the \#foo style, since the \# prefix stands out among the text,
better than \{..} does, and because # character is a better signal of
an operator than {.

create operator \{add_point}
(function = box_add, leftarg = box, rightarg = point);
create table test(a box);
insert into test values('((0,0),(1,1))'), ('((0,0),(2,1))');
select a as original, a \{add_point} '(1,1)' as modified from test;
drop operator \{add_point}(box, point);

Best regards,
Gurjeet
http://Gurje.et
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 1017f2eed1..c5b8562cb5 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -31,6 +31,7 @@
 #include "catalog/pg_type.h"
 #include "miscadmin.h"
 #include "parser/parse_oper.h"
+#include "parser/scansup.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
@@ -79,6 +80,10 @@ validOperatorName(const char *name)
 	if (len == 0 || len >= NAMEDATALEN)
 		return false;
 
+	/* Is this a Named Operator? */
+	if (validNamedOperator(name))
+		return true;
+
 	/* Can't contain any invalid characters */
 	/* Test string here should match op_chars in scan.l */
 	if (strspn(name, "~!@#^&|`?+-*/%<>=") != len)
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index db8b0fe8eb..8587b82c8d 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -379,6 +379,16 @@ self			[,()\[\].;\:\+\-\*\/\%\^\<\>\=]
 op_chars		[\~\!\@\#\^\&\|\`\?\+\-\*\/\%\<\>\=]
 operator		{op_chars}+
 
+/*
+ * Named Operators, e.g. \{foo}
+ *
+ * {namedopfailed*} are error rules to avoid scanner backup when
+ * {namedop} fails to match its trailing tokens.
+ */
+namedop			\\\{{identifier}\}
+namedopfailed1	\\\{{identifier}
+namedopfailed2	\\\{
+
 /*
  * Numbers
  *
@@ -768,6 +778,23 @@ other			.
 }
 <>	{ yyerror("unterminated dollar-quoted string"); }
 
+{namedop}		{
+	SET_YYLLOC();
+	if (yyleng >= NAMEDATALEN)
+		yyerror("operator name too long");
+	/* XXX Should we support double-quoted, case sensitive names? */
+	yylval->str = downcase_identifier(yytext, yyleng, false, false);
+	return Op;
+}
+
+{namedopfailed1}	{
+	yyerror("unexpected token");
+}
+
+{namedopfailed2}	{
+	yyerror("unexpected token");
+}
+
 {xdstart}		{
 	SET_YYLLOC();
 	BEGIN(xd);
diff --git a/src/backend/parser/scansup.c b/src/backend/parser/scansup.c
index 602108a40f..05c46ae09e 100644
--- a/src/backend/parser/scansup.c
+++ b/src/backend/parser/scansup.c
@@ -125,3 +125,70 @@ scanner_isspace(char ch)
 		return true;
 	return false;
 }
+
+/*
+ * validNamedOperator() -- return true if name adheres to the scanner rule
+ * {namedop}
+ */
+bool
+validNamedOperator(const char *name)
+{
+	size_t	len = strlen(name);
+	bool	valid_identifier;
+	char   *tmp;
+
+	if (len < 4 || len >= NAMEDATALEN)
+	   return false;
+
+	if (name[0] != '\\' || name[1] != '{' || name[len-1] != '}')
+		return false;
+
+	tmp = pstrdup(name);
+
+	// Disregard the delimiters
+	tmp[len-1] = '\0';
+	valid_identifier = validIdentifier(tmp + 2);
+	pfree(tmp);
+
+	return valid_identifier;
+}
+
+/*
+ * validIdentifier() -- return true if name adheres to the scanner rule
+ * {identifier}
+ *
+ * Note: this function does not check if the identifier length
+ * is less than NAMEDATALEN.
+ */
+bool
+validIdentifier(const char *name)
+{
+	uin

Re: Named Operators

2023-01-20 Thread Gurjeet Singh

On Fri, Jan 20, 2023 at 9:32 AM Ted Yu  wrote:
>
> Since `validIdentifier` doesn't modify the contents of `name` string, it 
> seems that there is no need to create `tmp` string in `validNamedOperator`.
> You can pass the start and end offsets into the string (name) as second and 
> third parameters to `validIdentifier`.

Thanks for reviewing the patch!

I was making a temporary copy of the string, since I had to modify it
before the validation, whereas the callee expects a `const char*`. I
agree that the same check can be done with more elegance, while
eliminating the temporary allocation. Please find the updated patch
attached.

Instead of passing the start and end of region I want to check, as
suggested, I'm now passing just the length of the string I want
validated. But I think that's for the better, since it now aligns with
the comment that validIdentifier() does not check if the passed string
is shorter than NAMEDATALEN.

Best regards,
Gurjeet
http://Gurje.et

named_operators_v4.patch
Description: Binary data

Re: generate_series for timestamptz and time zone problem

2023-01-29 Thread Gurjeet Singh

On Sat, Nov 12, 2022 at 10:44 AM Tom Lane  wrote:
> However, what it shouldn't be is part of this patch.  It's worth
> pushing it separately to have a record of that decision.  I've
> now done that, so you'll need to rebase to remove that delta.
>
> I looked over the v5 patch very briefly, and have two main
> complaints:
>
> * There's no documentation additions.  You can't add a user-visible
> function without adding an appropriate entry to func.sgml.
>
> * I'm pretty unimpressed with the whole truncate-to-interval thing
> and would recommend you drop it.  I don't think it's adding much
> useful functionality beyond what we can already do with the existing
> date_trunc variants; and the definition seems excessively messy
> (too many restrictions and special cases).

Please see attached v6 of the patch.

The changes since v5 are:
.) Rebased and resolved conflicts caused by commit 533e02e92.
.) Removed code and tests related to new date_trunc() functions, as
suggested by Tom.
.) Added 3 more variants to accompany with date_add(tstz, interval, zone).
date_add(tstz, interval)
date_subtract(tstz, interval)
date_subtract(tstz, interval, zone)

.) Eliminate duplication of code; use common function to implement
generate_series_timestamptz[_at_zone]() functions.
.) Fixed bug where in one of the new code paths,
generate_series_timestamptz_with_zone(),  did not perform
TIMESTAMP_NOT_FINITE() check.
.) Replaced some DirectFunctionCall?() with direct calls to the
relevant *_internal() function; should be better for performance.
.) Added documentation all 5 functions (2 date_add(), 2
date_subtract(), 1 overloaded version of generate_series()).

I'm not sure of the convention around authorship. But since this was
not an insignificant amount of work, would this patch be considered as
co-authored by Przemyslaw and myself? Should I add myself to Authors
field in the Commitfest app?

Hi Przemyslaw,

I started working on this patch based on Tom's review a few days
ago, since you hadn't responded in a while, and I presumed you're not
working on this anymore. I should've consulted with/notified you of my
intent before starting to work on it, to avoid duplication of work.
Sorry if this submission obviates any work you have in progress.
Please feel free to provide your feedback on the v6 of the patch.

Best regards,
Gurjeet
http://Gurje.et

generate_series_with_timezone.v6.patch
Description: Binary data

Re: generate_series for timestamptz and time zone problem

2023-01-30 Thread Gurjeet Singh

On Mon, Jan 30, 2023 at 4:07 PM Tom Lane  wrote:
> Gurjeet Singh  writes:
> > [ generate_series_with_timezone.v6.patch ]
>
> The cfbot isn't terribly happy with this.  It looks like UBSan
> is detecting some undefined behavior.  Possibly an uninitialized
> variable?

It was the classical case of out-of-bounds access. I was trying to
access 4th argument, even in the case where the 3-argument variant of
generate_series() was called.

Please see attached v7 of the patch. It now checks PG_NARGS() before
accessing the optional parameter.

This mistake would've been caught early if there were assertions
preventing access beyond the number of arguments passed to the
function. I'll send the assert_enough_args.patch, that adds these
checks, in a separate thread to avoid potentially confusing cfbot.

Best regards,
Gurjeet
http://Gurje.et
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e09e289a43..aa15407936 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9231,6 +9231,22 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
 

 
+   
+
+ date_add ( timestamp with time zone, interval , text  )
+ timestamp with time zone
+
+
+ Add interval to a timestamp with time zone value,
+ at the time zone specified by the third parameter. The time zone value
+ defaults to current  setting.
+
+
+ date_add('2021-10-31 00:00:00+02'::timestamptz, '1 day'::interval, 'Europe/Warsaw')
+ 2021-10-31 23:00:00
+
+   
+

 
  date_bin ( interval, timestamp, timestamp )
@@ -9278,6 +9294,22 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
 

 
+   
+
+ date_subtract ( timestamp with time zone, interval , text  )
+ timestamp with time zone
+
+
+ Subtract interval from a timestamp with time zone value,
+ at the time zone specified by the third parameter. The time zone value
+ defaults to the current  setting.
+
+
+ date_subtract('2021-10-31 00:00:00+02'::timestamptz, '1 day'::interval, 'Europe/Warsaw')
+ 2021-10-29 22:00:00
+
+   
+

 
  
@@ -21968,13 +22000,14 @@ AND
 setof timestamp


-generate_series ( start timestamp with time zone, stop timestamp with time zone, step interval )
+generate_series ( start timestamp with time zone, stop timestamp with time zone, step interval , timezone text  )
 setof timestamp with time zone


 Generates a series of values from start
 to stop, with a step size
-of step.
+of step. timezone
+defaults to the current  setting.

   
  
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 47e059a409..bd85f6421e 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -69,6 +69,7 @@ typedef struct
 	TimestampTz finish;
 	Interval	step;
 	int			step_sign;
+	pg_tz	   *attimezone;
 } generate_series_timestamptz_fctx;
 
 
@@ -78,6 +79,8 @@ static bool AdjustIntervalForTypmod(Interval *interval, int32 typmod,
 	Node *escontext);
 static TimestampTz timestamp2timestamptz(Timestamp timestamp);
 static Timestamp timestamptz2timestamp(TimestampTz timestamp);
+static pg_tz* lookup_timezone(text *zone);
+static Datum generate_series_timestamptz_internal(FunctionCallInfo fcinfo);
 
 
 /* common code for timestamptypmodin and timestamptztypmodin */
@@ -550,6 +553,54 @@ parse_sane_timezone(struct pg_tm *tm, text *zone)
 	return tz;
 }
 
+/*
+ * Look up the requested timezone (see notes in timestamptz_zone()).
+ */
+static pg_tz *
+lookup_timezone(text *zone)
+{
+	char		tzname[TZ_STRLEN_MAX + 1];
+	char	   *lowzone;
+	int			type,
+dterr,
+val;
+	pg_tz	   *tzp;
+
+	DateTimeErrorExtra extra;
+
+	text_to_cstring_buffer(zone, tzname, sizeof(tzname));
+
+	/* DecodeTimezoneAbbrev requires lowercase input */
+	lowzone = downcase_truncate_identifier(tzname,
+		   strlen(tzname),
+		   false);
+
+	dterr = DecodeTimezoneAbbrev(0, lowzone, &type, &val, &tzp, &extra);
+	if (dterr)
+		DateTimeParseError(dterr, &extra, NULL, NULL, NULL);
+
+	if (type == TZ || type == DTZ)
+	{
+		/* fixed-offset abbreviation, get a pg_tz descriptor for that */
+		tzp = pg_tzset_offset(-val);
+	}
+	else if (type == DYNTZ)
+	{
+		/* dynamic-offset abbreviation, use its referenced timezone */
+	}
+	else
+	{
+		/* try it as a full zone name */
+		tzp = pg_tzset(tzname);
+		if (!tzp)
+			ereport(ERROR,
+	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+	 errmsg("time zone \"%s\" not recognized", tzname)));

Assert fcinfo has enough args before allowing parameter access (was: Re: generate_series for timestamptz and time zone problem)

2023-01-30 Thread Gurjeet Singh

On Mon, Jan 30, 2023 at 11:50 PM Gurjeet Singh  wrote:
> It was the classical case of out-of-bounds access.

> This mistake would've been caught early if there were assertions
> preventing access beyond the number of arguments passed to the
> function. I'll send the assert_enough_args.patch, that adds these
> checks, in a separate thread to avoid potentially confusing cfbot.

Please see attached the patch to that ensures we don't accidentally
access more parameters than that are passed to a SQL callable
function.

Best regards,
Gurjeet
http://Gurje.et
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index b120f5e7fe..a445ac56b9 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -206,7 +206,7 @@ extern void fmgr_symbol(Oid functionId, char **mod, char **fn);
  * If function is not marked "proisstrict" in pg_proc, it must check for
  * null arguments using this macro.  Do not try to GETARG a null argument!
  */
-#define PG_ARGISNULL(n)  (fcinfo->args[n].isnull)
+#define PG_ARGISNULL(n)  (AssertMacro(n < PG_NARGS()), fcinfo->args[n].isnull)
 
 /*
  * Support for fetching detoasted copies of toastable datatypes (all of
@@ -265,7 +265,7 @@ extern struct varlena *pg_detoast_datum_packed(struct varlena *datum);
 
 /* Macros for fetching arguments of standard types */
 
-#define PG_GETARG_DATUM(n)	 (fcinfo->args[n].value)
+#define PG_GETARG_DATUM(n)	 (AssertMacro(n < PG_NARGS()), fcinfo->args[n].value)
 #define PG_GETARG_INT32(n)	 DatumGetInt32(PG_GETARG_DATUM(n))
 #define PG_GETARG_UINT32(n)  DatumGetUInt32(PG_GETARG_DATUM(n))
 #define PG_GETARG_INT16(n)	 DatumGetInt16(PG_GETARG_DATUM(n))

pg_reload_conf() synchronously

2022-11-04 Thread Gurjeet Singh

In an internal conversation it was seen that for some tests that want
to enforce a behaviour, a behaviour that is controlled by a GUC, we
_need_ to perform a sleep for an arbitrary amount of time.
Alternatively, executing the rest of the test on a new connection also
helps to get the expected behaviour. Following is a sample snippet of
such a test.

ALTER SYSTEM SET param TO value;
SELECT pg_reload_conf();
-- Either pg_sleep(0.1) or \connect here for next command to behave as expected.
ALTER ROLE ... PASSWORD '...';

This is because of the fact that the SIGHUP, sent to Postmaster by
this backend, takes some time to get back to the issuing backend.

Although a pg_sleep() call works to alleviate the pain in most cases,
it does not provide any certainty. Following the Principle Of Least
Astonishment, we want a command that loads the configuration
_synchronously_, so that the subsequent commands from the client can
be confident that the requisite parameter changes have taken effect.

The attached patch makes the pg_reload_conf() function set
ConfigReloadPending to true, which will force the postgres main
command-processing loop to process and apply config changes _before_
executing the next command.

The only downside I can think of this approach is that it _might_
cause the issuing backend to process the config file twice; once after
it has processed the current command, and another time when the SIGHUP
signal comes from the Postmaster. If the SIGHUP signal from the
Postmaster comes before the end of current command, then the issuing
backend will process the config only once, as before the patch. In any
case, this extra processing of the config seems harmless, and the
benefit outweighs the extra processing done.

The alternate methods that I considered (see branch reload_my_conf at
[2])  were to either implement the SQL command RELOAD CONFIGURATION [
FOR ALL ], or to create an overloaded version of function
pg_reload_conf(). But both those approaches had much more significant
downsides, like additional GRANTs, etc.

There have been issues identified in the past (see [1]) that possibly
may be alleviated by this approach of applying config changes
synchronously.

[1]: https://www.postgresql.org/message-id/2138662.1623460441%40sss.pgh.pa.us
[2]: https://github.com/gurjeet/postgres/tree/reload_my_conf

Best regards,
Gurjeet
http://Gurje.et


reload_conf_synchronouly.patch
Description: Binary data

Automatic notification for top transaction IDs

2021-06-22 Thread Gurjeet Singh

I came across this thread [1] to disallow canceling a transaction not
yet confirmed by a synchronous replica. I think my proposed patch
might help that case as well, hence adding all involved in that thread
to BCC, for one-time notification.

As mentioned in that thread, when sending a cancellation signal, the
client cannot be sure if the cancel signal was honored, and if the
transaction was cancelled successfully. In the attached patch, the
backend emits a NotificationResponse containing the current full
transaction id. It does so only if the relevant GUC is enabled, and
when the top-transaction is being assigned the ID.

This information can be useful to the client, when:
i) it wants to cancel a transaction _after_ issuing a COMMIT, and
ii) it wants to check the status of its transaction that it sent
COMMIT for, but never received a response (perhaps because the server
crashed).

Additionally, this information can be useful for middleware, like
Transaction Processing Monitors, which can now transparently (without
any change in application code) monitor the status of transactions (by
watching for the transaction status indicator in the ReadyForQuery
protocol message). They can use the transaction ID from the
NotificationResponse to open a watcher, and on seeing either an 'E' or
'I' payload in subsequent ReadyForQuery messages, close the watcher.
On server crash, or other adverse events, they can then use the
transaction IDs still being watched to check status of those
transactions, and take appropriate actions, e.g. retry any aborted
transactions.

We cannot use the elog() mechanism for this notification because it is
sensitive to the value of client_min_messages. Hence I used the NOTIFY
infrastructure for this message. I understand that this usage violates
some expectations as to how NOTIFY messages are supposed to behave
(see [2] below), but I think these are acceptable violations; open to
hearing if/why this might not be acceptable, and any possible
alternatives.

I'm not very familiar with the parallel workers infrastructure, so the
patch is missing any consideration for those.

Reviews welcome.

[1]: subject was: Re: Disallow cancellation of waiting for synchronous
replication
thread:
https://www.postgresql.org/message-id/flat/C1F7905E-5DB2-497D-ABCC-E14D4DEE506C%40yandex-team.ru

[2]:
At present, NotificationResponse can only be sent outside a
transaction, and thus it will not occur in the middle of a
command-response series, though it might occur just before ReadyForQuery.
It is unwise to design frontend logic that assumes that, however.
Good practice is to be able to accept NotificationResponse at any
point in the protocol.

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

notify_xid.patch
Description: Binary data

Automatic notification of top transaction IDs

2021-06-30 Thread Gurjeet Singh

(Re-sending this email, because the Commitfest app mistakenly [3]
considered previous email [4] to be part of the old thread, whereas it
should not be considered that way)

I'm not very familiar with the parallel workers infrastructure, so the
patch is missing any consideration for those.

Reviews welcome.

[1]: subject was: Re: Disallow cancellation of waiting for synchronous
replication
thread:
https://www.postgresql.org/message-id/flat/C1F7905E-5DB2-497D-ABCC-E14D4DEE506C%40yandex-team.ru

[3]:

See Emails section in https://commitfest.postgresql.org/33/3198/

The email [4] is considered a continuation of a previous thread, _and_
the 'Latest attachment' entry points to a different email, even though
my email [4] contained a patch.

[4]:
https://www.postgresql.org/message-id/CABwTF4VS+HVm11XRE_Yv0vGmG=5kpYdx759RyJEp9F+fiLTU=q...@mail.gmail.com

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

Re: Automatic notification of top transaction IDs

2021-06-30 Thread Gurjeet Singh

The proposed patch is attached.

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

On Wed, Jun 30, 2021 at 5:56 PM Gurjeet Singh  wrote:
>
> (Re-sending this email, because the Commitfest app mistakenly [3]
> considered previous email [4] to be part of the old thread, whereas it
> should not be considered that way)
>
> I came across this thread [1] to disallow canceling a transaction not
> yet confirmed by a synchronous replica. I think my proposed patch
> might help that case as well, hence adding all involved in that thread
> to BCC, for one-time notification.
>
> As mentioned in that thread, when sending a cancellation signal, the
> client cannot be sure if the cancel signal was honored, and if the
> transaction was cancelled successfully. In the attached patch, the
> backend emits a NotificationResponse containing the current full
> transaction id. It does so only if the relevant GUC is enabled, and
> when the top-transaction is being assigned the ID.
>
> This information can be useful to the client, when:
> i) it wants to cancel a transaction _after_ issuing a COMMIT, and
> ii) it wants to check the status of its transaction that it sent
> COMMIT for, but never received a response (perhaps because the server
> crashed).
>
> Additionally, this information can be useful for middleware, like
> Transaction Processing Monitors, which can now transparently (without
> any change in application code) monitor the status of transactions (by
> watching for the transaction status indicator in the ReadyForQuery
> protocol message). They can use the transaction ID from the
> NotificationResponse to open a watcher, and on seeing either an 'E' or
> 'I' payload in subsequent ReadyForQuery messages, close the watcher.
> On server crash, or other adverse events, they can then use the
> transaction IDs still being watched to check status of those
> transactions, and take appropriate actions, e.g. retry any aborted
> transactions.
>
> We cannot use the elog() mechanism for this notification because it is
> sensitive to the value of client_min_messages. Hence I used the NOTIFY
> infrastructure for this message. I understand that this usage violates
> some expectations as to how NOTIFY messages are supposed to behave
> (see [2] below), but I think these are acceptable violations; open to
> hearing if/why this might not be acceptable, and any possible
> alternatives.
>
> I'm not very familiar with the parallel workers infrastructure, so the
> patch is missing any consideration for those.
>
> Reviews welcome.
>
> [1]: subject was: Re: Disallow cancellation of waiting for synchronous
> replication
> thread: 
> https://www.postgresql.org/message-id/flat/C1F7905E-5DB2-497D-ABCC-E14D4DEE506C%40yandex-team.ru
>
> [2]:
>  At present, NotificationResponse can only be sent outside a
>  transaction, and thus it will not occur in the middle of a
>  command-response series, though it might occur just before ReadyForQuery.
>  It is unwise to design frontend logic that assumes that, however.
>  Good practice is to be able to accept NotificationResponse at any
>  point in the protocol.
>
> [3]:
>
> See Emails section in https://commitfest.postgresql.org/33/3198/
>
> The email [4] is considered a continuation of a previous thread, _and_
> the 'Latest attachment' entry points to a different email, even though
> my email [4] contained a patch.
>
> [4]: 
> https://www.postgresql.org/message-id/CABwTF4VS+HVm11XRE_Yv0vGmG=5kpYdx759RyJEp9F+fiLTU=q...@mail.gmail.com
>
> Best regards,
> --
> Gurjeet Singh http://gurjeet.singh.im/


notify_xid.patch
Description: Binary data

Slightly improve initdb --sync-only option's help message

2021-07-06 Thread Gurjeet Singh

When reading the output of `initdb --help` I could not clearly
understand what the purpose of the --sync-only option was, until I
read the documentation of initdb.

  -S, --sync-only   only sync data directory

Perhaps the confusion was caused by the fact that sync(hronization)
means different things in different contexts, and many of those
contexts apply to databases, and to data directories; time sync, data
sync, replica sync, etc.

I think it would be helpful if the help message was slightly more
descriptive. Some options:

Used in patch:
 only sync data directory; does not modify any data

To match the wording of --sync-only option:
write contents of data directory to disk; helpful after --no-sync option

Clearly specify the system operation used for the option
perform fsync on data directory; helpful after --no-sync option

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/


v1-0001-Explicitly-state-that-sync-only-does-not-modify-d.patch
Description: Binary data

Warn if initdb's --sync-only option is mixed with other options

2021-07-06 Thread Gurjeet Singh

When reading through code for my previous patch [1] I realized that
initdb does *not* warn users that it ignores all other options (except
-D/--pgdata) if the --sync-only option is used.

I'm not able to come up with an exact situation to prove this, but
this behaviour seems potentially dangerous. The user might mix the
--sync-only option with other options, but would be extremely
surprised if those other options didn't take effect.

I _think_ we should throw an error if the user specifies any options
that are being ignored. But an error might break someone's automation
(perhaps for their own good), since the current behaviour has been in
place for a very long time, so I'm willing to settle for at least a
warning in such a case.

[1]:
Slightly improve initdb --sync-only option's help message
https://www.postgresql.org/message-id/CABwTF4U6hbNNE1bv%3DLxQdJybmUdZ5NJQ9rKY9tN82NXM8QH%2BiQ%40mail.gmail.com

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/


v1-0001-Warn-if-sync-only-is-used-with-other-options.patch
Description: Binary data

Patch: Don't set LoadedSSL unless secure_initialize succeeds

2022-05-21 Thread Gurjeet Singh

The initialization in PostmasterMain() blindly turns on LoadedSSL,
irrespective of the outcome of secure_initialize(). I don't think
that's how it should behave, primarily because of the pattern followed
by the other places that call secure_initialize().

This patch makes PostmasterMain() behave identical to other places
(SIGHUP handler, and SubPostmasterMain()) where LoadedSSL is turned on
after checking success of secure_initialize() call. Upon failure of
secure_initialize(), it now emits a log message, instead of setting
LoadedSSL to true.

Best regards,
Gurjeet
http://Gurje.et


LoadedSSL.patch
Description: Binary data

Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

2022-05-25 Thread Gurjeet Singh

On Sun, May 22, 2022 at 12:17 AM Daniel Gustafsson  wrote:
> > On 22 May 2022, at 08:41, Gurjeet Singh  wrote:
>
> > The initialization in PostmasterMain() blindly turns on LoadedSSL,
> > irrespective of the outcome of secure_initialize().
>
> This call is invoked with isServerStart set to true so any error in
> secure_initialize should error out with ereport FATAL (in be_tls_init()).  
> That
> could be explained in a comment though, which is currently isn't.

That makes sense. I have attached a patch that adds a couple of lines
of comments explaining this at call-site.

> Did you manage to get LoadedSSL set to true without SSL having been properly
> initialized?

Fortunately, no. I'm trying to add a new network protocol, and caught
this inconsistency while reading/adapting the code.

If a committer sees some value in it, in the attached patch I have
also attempted to improve the readability of the code a little bit in
startup-packet handling, and in SSL functions. These are purely
cosmetic changes, but I think they reduce repetition, and improve
readability, by quite a bit. For example, every ereport call evaluates
the same 'isServerStart ? FATAL : LOG', over and over again; replacing
this with variable 'logLevel' reduces cognitive load for the reader.
And I've replace one 'goto retry1' with a 'while' loop, like the
GASSAPI does it below that occurrence.

There's an symmetry, almost a diametric opposition, between how SSL
initialization error is treated when it occurs during server startup,
versus when the error occurs during a reload/SIGHUP. During startup an
error in SSL initialization leads to FATAL, whereas during a SIGHUP
it's merely a LOG message.

I found this difference in treatment of SSL initialization errors
quite bothersome, and there is no ready explanation for this. Either a
properly initialized SSL stack is important for server operation, or
it is not. What do we gain by letting the server operate normally
after a reload that failed to initialize SSL stack. Conversely, why do
we kill the server during startup on SSL initialization error, when
it's okay to operate normally after a reload that is unable to
initialize SSL.

I have added a comment to be_tls_init(), which I hope explains this
difference in treatment of errors. I have also added comments to
be_tls_init(), explaining why we don't destroy/free the global
SSL_context variable in case of an error in re-initialization of SSL;
it's not just an optimization, it's essential to normal server
operation.

Best regards,
Gurjeet
http://Gurje.et

readability_improvements-ssl-startup_packet.patch
Description: Binary data

Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

2022-05-25 Thread Gurjeet Singh

On Mon, May 23, 2022 at 8:51 PM Tom Lane  wrote:
>
> Daniel Gustafsson  writes:
> >> On 22 May 2022, at 08:41, Gurjeet Singh  wrote:
> >> The initialization in PostmasterMain() blindly turns on LoadedSSL,
> >> irrespective of the outcome of secure_initialize().
>
> > This call is invoked with isServerStart set to true so any error in
> > secure_initialize should error out with ereport FATAL (in be_tls_init()).  
> > That
> > could be explained in a comment though, which is currently isn't.
>
> The comments for secure_initialize() and be_tls_init() both explain
> this already.

The comments above secure_initialize() do, but there are no comments
above be_tls_init(), and nothing in there attempts to explain the
FATAL vs. LOG difference.

I think it doesn't hurt, and may actively help, if we add a comment at
the call-site just alluding to the fact that the function call will
not return in case of an error, and that it's safe to assume certain
state has been initialized if the function returns.

The comments *inside* be_tls_init() attempt to explain allocation of a
new SSL_context, but end up making it sound like it's an optimization
to prevent memory leak. In the patch proposed a few minutes ago in
this thread, I have tried to explain above be_tls_init() the error
handling  behavior, as well as the reason to retain the active
SSL_context, if any.

> It's not great that be_tls_init() implements two different error
> handling behaviors, perhaps.  One could imagine separating those.
> But we've pretty much bought into such messes with the very fact
> that elog/ereport sometimes return and sometimes not.

I don't find the dual mode handling startling; I feel it's common in
Postgres code, but it's been a while since I've touched it.

What I would love to see improved around ereport() calls in SSL
functions would be to shrink the 'ereport(); goto error;' pattern into
one statement, so that we don't introduce an accidental "goto fail"
bug [1].

[1]: 
https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/

Best regards,
Gurjeet
http://Gurje.et

Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

2022-05-25 Thread Gurjeet Singh

On Wed, May 25, 2022 at 10:05 PM Gurjeet Singh  wrote:
> I have added a comment to be_tls_init(), which I hope explains this
> difference in treatment of errors. I have also added comments to
> be_tls_init(), explaining why we don't destroy/free the global
> SSL_context variable in case of an error in re-initialization of SSL;
> it's not just an optimization, it's essential to normal server
> operation.

Please see attached patch that reverts the unintentional removal of a
comment in be_tls_init(). Forgot to put that comment back in after my
edits.

Best regards,
Gurjeet
http://Gurje.et


revert-unintentional-deletion.patch
Description: Binary data

Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

On Thu, May 26, 2022 at 12:16 PM Tom Lane  wrote:
> Gurjeet Singh  writes:
> > On Mon, May 23, 2022 at 8:51 PM Tom Lane  wrote:
> >> The comments for secure_initialize() and be_tls_init() both explain
> >> this already.
>
> > The comments above secure_initialize() do, but there are no comments
> > above be_tls_init(), and nothing in there attempts to explain the
> > FATAL vs. LOG difference.
>
> I was looking at the comments in libpq-be.h:
>
> /*
>  * Initialize global SSL context.
>  *
>  * If isServerStart is true, report any errors as FATAL (so we don't return).
>  * Otherwise, log errors at LOG level and return -1 to indicate trouble,
>  * preserving the old SSL state if any.  Returns 0 if OK.
>  */
> extern int  be_tls_init(bool isServerStart);
>
> It isn't our usual practice to put such API comments with the extern
> rather than the function definition,

Yep, and I didn't notice these comments, or even bothered to look at
the extern declaration, precisely because my knowledge of Postgres
coding convention told me the comments are supposed to be on the
function definition.

> so maybe those comments in libpq-be.h
> should be moved to their respective functions?  In any case, I'm not
> excited about having three separate comments covering the same point.

By 3 locations, I suppose you're referring to the definition of
secure_initialize(), extern declaration of be_tls_init(), and the
definition of be_tls_init().

The comment on the extern declaration does not belong there, so that
one definitely needs go. The other two locations need descriptive
comments, even if they might sound duplicate. Because the one on
secure_initialize() describes the abstraction's expectations, and the
one on be_tls_init() should refer to it, and additionally mention any
implementation details.

Best regards,
Gurjeet
http://Gurje.et

Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

On Thu, May 26, 2022 at 1:00 PM Robert Haas  wrote:
>
> On Thu, May 26, 2022 at 1:05 AM Gurjeet Singh  wrote:
> > There's an symmetry, almost a diametric opposition, between how SSL

I meant "an asymmetry".

> > initialization error is treated when it occurs during server startup,
> > versus when the error occurs during a reload/SIGHUP. During startup an
> > error in SSL initialization leads to FATAL, whereas during a SIGHUP
> > it's merely a LOG message.
> >
> > I found this difference in treatment of SSL initialization errors
> > quite bothersome, and there is no ready explanation for this. Either a
> > properly initialized SSL stack is important for server operation, or
> > it is not. What do we gain by letting the server operate normally
> > after a reload that failed to initialize SSL stack. Conversely, why do
> > we kill the server during startup on SSL initialization error, when
> > it's okay to operate normally after a reload that is unable to
> > initialize SSL.
>
> I think you're overreacting to a behavior that isn't really very surprising.

The behaviour is not surprising. I developed those opposing views as I
was reading the code. And I understood the behaviour after I was done
reading the code. But I was irked that it wasn't clearly explained
somewhere nearby in code. Hence my proposal:

> > I have added a comment to be_tls_init(), which I hope explains this
> > difference in treatment of errors.

> So I don't really know what behavior, other than what is actually
> implemented, would be reasonable.

I just wasn't happy about the fact that I had wasted time trying to
find holes (security holes!) in the behaviour. So my proposal is to
improve the docs/comments about this behaviour, and not the behaviour
itself.

Best regards,
Gurjeet
http://Gurje.et

Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

On Thu, May 26, 2022 at 2:40 PM Tom Lane  wrote:
>
> Robert Haas  writes:
> > I think you're overreacting to a behavior that isn't really very surprising.
>
> > If we don't initialize SSL the first time, we don't have a working SSL
> > stack. If we didn't choose to die at that point, we'd be starting up a
> > server that could not accept any SSL connections. I don't think users
> > would like that.
>
> > If we don't *reinitialize* it, we *do* have a working SSL stack. We
> > haven't been able to load the updated configuration, but we still have
> > the old one. We could fall over and die anyway, but I don't think
> > users would like that either. People don't expect 'pg_ctl reload' to
> > kill off a working server, even if the new configuration is bad.
>
> The larger context here is that this is (or at least is supposed to be)
> exactly the same as our reaction to any other misconfiguration: die if
> it's detected at server start, but if it's detected during a later SIGHUP
> reload, soldier on with the known-good previous settings.  I believe
> that's fairly well documented already.

This distinction (of server startup vs. reload) is precisely what I
think should be conveyed and addressed in the comments of functions
responsible for (re)initialization of resources. Such a comment,
specifically calling out processing/logging/error-handling differences
between startup and reload, would've definitely helped when I was
trying to understand the code, and trying to figure out the different
contexts these functions may be executed in. The fact that
ProcessStartupPacket() function calls these functions, and then also
calls itself recursively, made understanding the code and intent much
harder.

And since variable/parameter/function names also convey intent, their
naming should also be as explicit as possible, rather than being
vague.

Calling variables/parameters 'isServerStart' leaves so much to
interpretation; how many and what other cases is the code called in
when isServerStart == false? I think a better scheme would've been to
name the parameter as 'reason', and use an enum/constant to convey
that there are exactly 2 higher-level cases that the code is called in
context of: enum InitializationReason { ServerStartup, ServerReload }.

In these functions, it's not important to know the distinction of
whether the server is starting-up vs. already running (or whatever
other states the server may be in) , instead it's important to know
the distinction of whether the server is starting-up or being
reloaded; other states of the server operation, if any, do not matter
here.

Best regards,
Gurjeet
http://Gurje.et

Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

On Thu, May 26, 2022 at 4:13 PM Tom Lane  wrote:
> Gurjeet Singh  writes:
> > On Thu, May 26, 2022 at 12:16 PM Tom Lane  wrote:
> >> so maybe those comments in libpq-be.h
> >> should be moved to their respective functions?  In any case, I'm not
> >> excited about having three separate comments covering the same point.
>
> > By 3 locations, I suppose you're referring to the definition of
> > secure_initialize(), extern declaration of be_tls_init(), and the
> > definition of be_tls_init().
>
> No, I was counting the third comment as the one you proposed to add to
> secure_initialize's caller.

I think a comment at that call-site is definitely warranted. Consider
the code as it right now...

if (EnableSSL)
{
(void) secure_initialize(true);
LoadedSSL = true;
}

... as a first-time reader.

Reader> This is an important piece of code, not just because it is
called from PostmasterMain(), early in the startup process, but also
because it deals with SSL; it has 'SSL' and 'secure' plastered all
over it. But wait, they don't care what the outcome of this important
function call is??!! I might not have paid much attention to it, but
the call is adorned with '(void)', which (i) attracts my attention
more, and (ii) why are they choosing to throw away the result of such
important function call?! And then, they declare SSL has been
"Loaded"... somebody committed half-written code! Perhaps they we in a
hurry. Perhaps this is a result of an automatic git-merge gone wrong.
Let me dig through the code and see if I can find a vulnerability.
 Duh, there's nothing wrong here. .

Now, consider the same code, and the ensuing thought-process of the reader:

if (EnableSSL)
{
/* Any failure during SSL initialization here will result in
FATAL error. */
(void) secure_initialize(true);
/* ... so here we know for sure that SSL was successfully
initialized. */
LoadedSSL = true;
}

Reader> This is an important piece of code, not just because it is
called from PostmasterMain(), early in the startup process, but also
because it deals with SSL; it has 'SSL' and 'secure' plastered all
over it. But wait, they don't care what the outcome of this important
function call is??!! That's okay, because the explanation in the
comment makes sense.  There's nothing wrong here. .

> I think it's not a great idea to add such
> comments to call sites, as they're very likely to not get maintained
> when somebody adjusts the API of the function.  (We have a hard enough
> time getting people to update the comments directly next to the
> function :-(.)

That's unfortunate. But I think we should continue to strive for more
maintainable, readable, extensible code.

> I think what we ought to do here is just move the oddly-placed comments
> in libpq-be.h to be adjacent to the function definitions, as attached.
> (I just deleted the .h comments for the GSSAPI functions, as they seem
> to have adequate comments in their .c file already.)

Please see if anything from my patches is usable. I did not get
clarity around startup vs. reload handling until my previous email, so
there may not be much of use in my patches. But I think a few words
mentioning the difference in resource (re)initialization during
startup vs reload would add a lot of value.

Best regards,
Gurjeet
http://Gurje.et

Re: Hardening PostgreSQL via (optional) ban on local file system access

2022-06-24 Thread Gurjeet Singh

On Fri, Jun 24, 2022 at 4:13 PM Andres Freund  wrote:
> On 2022-06-25 00:08:13 +0200, Hannu Krosing wrote:

> > 3) should this be back-patched (we can provide batches for all
> > supported PgSQL versions)
>
> Err, what?

Translation: Backpatching these changes to any stable versions will
not be acceptable (per the project versioning policy [1]), since these
changes would be considered new feature. These changes can break
installations, if released in a minor version.

[1]: https://www.postgresql.org/support/versioning/

Best regards,
Gurjeet
http://Gurje.et

Re: Hardening PostgreSQL via (optional) ban on local file system access

2022-06-24 Thread Gurjeet Singh

(fixed your top-posting)

On Fri, Jun 24, 2022 at 4:59 PM Hannu Krosing  wrote:
> On Sat, Jun 25, 2022 at 1:46 AM Gurjeet Singh  wrote:
> >
> > On Fri, Jun 24, 2022 at 4:13 PM Andres Freund  wrote:
> > > On 2022-06-25 00:08:13 +0200, Hannu Krosing wrote:
> >
> > > > 3) should this be back-patched (we can provide batches for all
> > > > supported PgSQL versions)
> > >
> > > Err, what?
> >
> > Translation: Backpatching these changes to any stable versions will
> > not be acceptable (per the project versioning policy [1]), since these
> > changes would be considered new feature. These changes can break
> > installations, if released in a minor version.
> >
> > [1]: https://www.postgresql.org/support/versioning/
>
> My understanding was that unless activated by admin these changes
> would change nothing.
>
> And they would be (borderline :) ) security fixes
>
> And the versioning policy link actually does not say anything about
> not adding features to older versions (I know this is the policy, just
> pointing out the info in not on that page).

I wanted to be sure before I mentioned it, and also because I've been
away from the community for a few years [1], so I too searched the
page for any relevant mentions of the word "feature" on that page.
While you're correct that the policy does not address/prohibit
addition of new features in minor releases, but the following line
from the policy comes very close to saying it, without actually saying
it.

> ... PostgreSQL minor releases fix only frequently-encountered bugs, security 
> issues, and data corruption problems to reduce the risk associated with 
> upgrading ...

Like I recently heard a "wise one" recently say: "oh those [Postgres]
docs are totally unclear[,] but they're technically correct".

BTW, the "Translation" bit was for folks new to, or not familiar with,
community and its lingo; I'm sure you already knew what Andres meant
:-)

[1]: I'll milk the "I've been away from the community for a few years"
excuse for as long as possible ;-)

Best regards,
Gurjeet
http://Gurje.et

Re: [PoC/RFC] Multiple passwords, interval expirations

2022-06-30 Thread Gurjeet Singh

I am planning on picking it up next week; right now picking up steam,
and reviewing a different, smaller patch.

At his behest, I had a conversation with Joshua (OP), and have his
support to pick up and continue working on this patch. I have a some
ideas of my own, on what this patch should do, but since I haven't
fully reviewed the (bulky) patch, I'll reserve my proposals until I
wrap my head around it.

Please expect some activity on this patch towards the end of next week.

BCC: Joshua's new work email.

Best regards,
Gurjeet
http://Gurje.et

On Wed, Jun 29, 2022 at 2:27 PM Stephen Frost  wrote:
>
> Greetings,
>
> On Wed, Jun 29, 2022 at 17:22 Jacob Champion  wrote:
>>
>> On 4/8/22 10:04, Joshua Brindle wrote:
>> > It's unclear if I will be able to continue working on this featureset,
>> > this email address will be inactive after today.
>>
>> I'm assuming the answer to this was "no". Is there any interest out
>> there to pick this up for the July CF?
>
>
> Short answer to that is yes, I’m interested in continuing this (though 
> certainly would welcome it if there are others who are also interested, and 
> may be able to bring someone else to help work on it too but that might be 
> more August / September time frame).
>
> Thanks,
>
> Stephen

Re: generate_series for timestamptz and time zone problem

2022-06-30 Thread Gurjeet Singh

On Tue, Jun 21, 2022 at 7:56 AM Przemysław Sztoch  wrote:
> There is another patch.
> It works, but one thing is wrongly done because I lack knowledge.

Thank you for continuing to work on it despite this being your first
time contributing, and despite the difficulties. I'll try to help as
much as I can.

> Where I'm using DirectFunctionCall3 I need to pass the timezone name, but I'm 
> using cstring_to_text and I'm pretty sure there's a memory leak here. But I 
> need help to fix this.
> I don't know how best to store the timezone in the generate_series context. 
> Please, help.

In Postgres code we generally don't worry about memory leaks (a few
caveats apply). The MemoryContext infrastructure (see aset.c) enables
us to be fast and loose with memory allocations. A good way to know if
you should be worried about your allocations, is to look in the
neighboring code, and see what does it do with the memory it
allocates.

I think your use of cstring_to_text() is safe.

> Please give me feedback on how to properly store the timezone name in the 
> function context structure. I can't finish my work without it.

The way I see it, I don't think you need to store the tz-name in the
function context structure, like you're currently doing. I think you
can remove the additional member from the
generate_series_timestamptz_fctx struct, and refactor your code in
generate_series_timestamptz() to work without it.; you seem to  be
using the tzname member almost as a boolean flag, because the actual
value you pass to DFCall3() can be calculated without first storing
anything in the struct.

> Additionally, I added a new variant of the date_trunc function that takes 
> intervals as an argument.
> It enables functionality similar to date_bin, but supports monthly, 
> quarterly, annual, etc. periods.
> In addition, it is resistant to the problems of different time zones and 
> daylight saving time (DST).

This addition is beyond the original scope (add TZ param), so I think
it would be considered a separate change/feature. But for now, we can
keep it in.

Although not necessary, it'd be nice to have changes that can be
presented as single units, be a patch of their own. If  you're
proficient with Git, can you please maintain each SQL-callable
function as a separate commit in your branch,  and use `git
format-patch` to generate a series for submission.

Can you please explain why you chose to remove the provolatile
attribute from the existing entry of date_trunc in pg_proc.dat.

It seems like you've picked/reused code from neighboring functions
(e.g. from timestamptz_trunc_zone()). Can you please see if you can
turn such code into a function, and  call the function, instead of
copying it.

Also, according to the comment at the top of pg_proc.dat,

 # Once upon a time these entries were ordered by OID.  Lately it's often
 # been the custom to insert new entries adjacent to related older entries.

So instead of adding your entries at the bottom of the file, please
each entry closer to an existing entry that's relevant to it.

I'm glad that you're following the advice on the patch-submission wiki
page [1]. When submitting a patch for committers' consideration,
though, the submission needs to cross quite a few hurdles. So have
prepared a markdown doc [2]. Let's fill in as much detail there as
possible, before we mark it 'Ready for Committer' in the CF app.

[1]: https://wiki.postgresql.org/wiki/Submitting_a_Patch
[2]: https://wiki.postgresql.org/wiki/Patch_Reviews

Best regards,
Gurjeet
http://Gurje.et

Fwd: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS

2022-07-20 Thread Gurjeet Singh

Moving the report from security to -hackers on Noah's advice. Since
the function(s) involved in the crash are not present in any of the
released versions, it is not considered a security issue.

I can confirm that this is reproducible on the latest commit on
master, 3c0bcdbc66. Below is the original analysis, followed by Noah's
analysis.

To be able to reproduce it, please note that perl support is required;
 hence `./configure --with-perl`.

The note about 'security concerns around on_plperl_init parameter',
below, refers to now-fixed issue, at commit 13d8388151.

Best regards,
Gurjeet
http://Gurje.et

Forwarded Conversation
Subject: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS
----

From: Gurjeet Singh 
Date: Mon, Jul 4, 2022 at 10:24 AM
To: Postgres Security 
Cc: Bossart, Nathan 


While poking at plperl's GUC in an internal discussion, I was able to
induce a crash (or an assertion failure in assert-enabled builds) as
an unprivileged user.

My investigation so far has revealed that the code path for the
following condition has never been tested, and because of this, when a
user tries to override an SUSET param via PGOPTIONS, Postgres tries to
perform a table lookup during process initialization. Because there's
no transaction in progress, and because this table is not in the
primed caches, we end up with code trying to dereference an
uninitialized  CurrentResourceOwner.

The condition:
User specifies PGOPTIONS"-c custom.param"
"custom.param" is used by an extension which is specified in
session_preload_libraries
The extension uses DefineCustom*Variable("custom.param", PGC_SUSET)
inside set_config_option()
  record->context == PGC_SUSET
  context == PGC_BACKEND
  calls pg_parameter_aclcheck()  -> eventually leads to
assertion-failure (or crash, when assertions are disabled)

See below for 1. How to reproduce, 2. Assertion failure stack, and 3.
Crash stack

When the user does not specify PGOPTIONS, the code in
define_custom_variable() returns prematurely, after a failed bsearch()
lookup, and hence avoids this bug.

I think similar crash can be  induced when the custom parameter is of
kind PGC_SU_BACKEND, because the code to handle that also invokes
pg_parameter_aclcheck(). Also, I believe the same condition would
arise if the extension is specified local_preload_libraries.

I haven't  been  able to think of an attack vector using this bug, but
it can be used to cause a denial-of-service by an authenticated user.
I'm sending this report to security list, instead of -hackers, out of
abundance of caution; please feel free to move it to -hackers, if it's
not considered a security concern.

I discovered this bug a couple of days ago, just before leaving on a
trip. But because of shortage of time over the weekend, I haven't been
able to dig deeper into it. Since I don't think I'll be able to spend
any significant time on it for at least another couple of days, I'm
sending this report without a patch or a proposed fix.

CC: Nathan, whose security concerns around on_plperl_init parameter
lead to this discovery.

[1]: How to reproduce

$ psql -c 'create user test'
CREATE ROLE

$ psql -c "alter system set session_preload_libraries='plperl'"
ALTER SYSTEM

$ # restart server

$ psql -c 'show session_preload_libraries'
 session_preload_libraries
---
 plperl
(1 row)

$ PGOPTIONS="-c plperl.on_plperl_init=" psql -U test
psql: error: connection to server on socket "/tmp/.s.psql.5432"
failed: server closed the connection unexpectedly
┆   This probably means the server terminated abnormally
before or while processing the request.


[2]:  Assertion failure stack

LOG:  database system is ready to accept connections
TRAP: FailedAssertion("IsTransactionState()", File:
"../../../../../../POSTGRES/src/backend/utils/cache/catcache.c", Line:
1209, P
ID: 199868)
postgres: test postgres [local]
startup(ExceptionalCondition+0xd0)[0x55e503a4e6c9]
postgres: test postgres [local] startup(+0x7e069b)[0x55e503a2a69b]
postgres: test postgres [local] startup(SearchCatCache1+0x3a)[0x55e503a2a56b]
postgres: test postgres [local] startup(SearchSysCache1+0xc1)[0x55e503a46fe4]
postgres: test postgres [local]
startup(pg_parameter_aclmask+0x6f)[0x55e50345f098]
postgres: test postgres [local]
startup(pg_parameter_aclcheck+0x2d)[0x55e50346039c]
postgres: test postgres [local] startup(set_config_option+0x450)[0x55e503a70727]
postgres: test postgres [local] startup(+0x829ce8)[0x55e503a73ce8]
postgres: test postgres [local]
startup(DefineCustomStringVariable+0xa4)[0x55e503a74306]
/home/vagrant/dev/POSTGRES_builds/add_tz_param/db/lib/postgresql/plperl.so(_PG_init+0xd7)[0x7fed3d845425]
postgres: test postgres [local] startup(+0x80cc50)[0x55e503a56c50]
postgres: test postgres [local] star

Re: Fwd: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS

2022-07-21 Thread Gurjeet Singh

On Thu, Jul 21, 2022 at 2:44 PM Tom Lane  wrote:
>
> Gurjeet Singh  writes:
> > While poking at plperl's GUC in an internal discussion, I was able to
> > induce a crash (or an assertion failure in assert-enabled builds) as
> > an unprivileged user.
> > My investigation so far has revealed that the code path for the
> > following condition has never been tested, and because of this, when a
> > user tries to override an SUSET param via PGOPTIONS, Postgres tries to
> > perform a table lookup during process initialization. Because there's
> > no transaction in progress, and because this table is not in the
> > primed caches, we end up with code trying to dereference an
> > uninitialized  CurrentResourceOwner.
>
> Right.  So there are basically two things we could do about this:
>
> 1. set_config_option could decline to call pg_parameter_aclcheck
> if not IsTransactionState(), instead failing the assignment.
> This isn't a great answer because it would result in disallowing
> GUC assignments that users might expect to work.
>
> 2. We could move process_session_preload_libraries() to someplace
> where a transaction is in progress -- probably, relocate it to
> inside InitPostgres().
>
> I'm inclined to think that #2 is a better long-term solution,
> because it'd allow you to session-preload libraries that expect
> to be able to do database access during _PG_init.  (Right now
> that'd fail with the same sort of symptoms seen here.)  But
> there's no denying that this might have surprising side-effects
> for extensions that weren't expecting such a change.
>
> It could also be reasonable to do both #1 and #2, with the idea
> that #1 might save us from crashing if there are any other
> code paths where we can reach that pg_parameter_aclcheck call
> outside a transaction.
>
> Thoughts?

I had debated just wrapping the process_session_preload_libraries()
call with a transaction, like Nathan's patch posted ealier on this
thread does. But I hesitated because of the sensitivity around the
order of operations/call during process initialization.

I like the idea of performing library initialization in
InitPostgres(), as it performs the first transaction of the
connection, and because of the libraries' ability to gin up new GUC
variables that might need special handling, and also if it allows them
to do database access.

I think anywhere after the 'PostAuthDelay' check in InitPostgres()
would be a good place to perform process_session_preload_libraries().
I'm inclined to invoke it as late as possible, before we commit the
transaction.

As for making set_config_option() throw an error if not in
transaction, I'm not a big fan of checks  that break the flow, and of
unrelated code showing up when reading a function. For a casual
reader, such a check for transaction would make for a jarring
experience; "why are we checking for active transaction in  the guts
of guc.c?", they might think. If anything, such an error should be
thrown from or below pg_parameter_aclcheck().

But I am not sure if it should be exposed as an error. A user
encountering that error is not at fault. Hence I believe an assertion
check is more suitable for catching code that invokes
set_config_option() outside a transaction.

Best regards,
Gurjeet
http://Gurje.et

Re: Fwd: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS

2022-07-21 Thread Gurjeet Singh

On Thu, Jul 21, 2022 at 3:29 PM Nathan Bossart  wrote:
>
> On Thu, Jul 21, 2022 at 05:44:11PM -0400, Tom Lane wrote:
> > Right.  So there are basically two things we could do about this:
> >
> > 1. set_config_option could decline to call pg_parameter_aclcheck
> > if not IsTransactionState(), instead failing the assignment.
> > This isn't a great answer because it would result in disallowing
> > GUC assignments that users might expect to work.
> >
> > 2. We could move process_session_preload_libraries() to someplace
> > where a transaction is in progress -- probably, relocate it to
> > inside InitPostgres().
> >
> > I'm inclined to think that #2 is a better long-term solution,
> > because it'd allow you to session-preload libraries that expect
> > to be able to do database access during _PG_init.  (Right now
> > that'd fail with the same sort of symptoms seen here.)  But
> > there's no denying that this might have surprising side-effects
> > for extensions that weren't expecting such a change.
> >
> > It could also be reasonable to do both #1 and #2, with the idea
> > that #1 might save us from crashing if there are any other
> > code paths where we can reach that pg_parameter_aclcheck call
> > outside a transaction.
> >
> > Thoughts?
>
> I wrote up a small patch along the same lines as #2 before seeing this
> message.  It simply ensures that process_session_preload_libraries() is
> called within a transaction.  I don't have a strong opinion about doing it
> this way versus moving this call somewhere else as you proposed, but I'd
> agree that #2 is a better long-term solution than #1.  AFAICT
> shared_preload_libraries, even with EXEC_BACKEND, should not have the same
> problem.
>
> I'm not sure whether we should be worried about libraries that are already
> creating transactions in their _PG_init() functions.  Off the top of my
> head, I don't recall seeing anything like that.  Even if it does impact
> some extensions, it doesn't seem like it'd be too much trouble to fix.
>
> diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
> index 8ba1c170f0..fd471d74a3 100644
> --- a/src/backend/tcop/postgres.c
> +++ b/src/backend/tcop/postgres.c
> @@ -4115,8 +4115,15 @@ PostgresMain(const char *dbname, const char *username)
>  /*
>   * process any libraries that should be preloaded at backend start (this
>   * likewise can't be done until GUC settings are complete)
> + *
> + * If the user provided a setting at session startup for a custom GUC
> + * defined by one of these libraries, we might need syscache access when
> + * evaluating whether she has permission to set it, so do this step 
> within
> + * a transaction.
>   */
> +StartTransactionCommand();
>  process_session_preload_libraries();
> +CommitTransactionCommand();
>
>  /*
>   * Send this backend's cancellation info to the frontend.

(none of the following is your patch's fault)

I don't think that is a good call-site for
process_session_preload_libraries(), because a library being loaded
can declare its own GUCs, hence I believe this should be called at
least before the call to BeginReportingGUCOptions().

If an extension creates a GUC with GUC_REPORT flag, it is violating
expectations. But since the DefineCustomXVariable() stack does not
prevent the callers from doing so, we must still honor the protocol
followed for all params with GUC_REPORT. And hence the

I think it'd be a good idea to ban the callers of
DefineCustomXVariable() from declaring their variable GUC_REPORT, to
ensure that only core code can define such variables.

Best regards,
Gurjeet
http://Gurje.et

Re: Fwd: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS

2022-07-21 Thread Gurjeet Singh

On Thu, Jul 21, 2022 at 4:35 PM Gurjeet Singh  wrote:
> I like the idea of performing library initialization in
> InitPostgres(), as it performs the first transaction of the
> connection, and because of the libraries' ability to gin up new GUC
> variables that might need special handling, and also if it allows them
> to do database access.
>
> I think anywhere after the 'PostAuthDelay' check in InitPostgres()
> would be a good place to perform process_session_preload_libraries().
> I'm inclined to invoke it as late as possible, before we commit the
> transaction.
>
> As for making set_config_option() throw an error if not in
> transaction, I'm not a big fan of checks  that break the flow, and of
> unrelated code showing up when reading a function. For a casual
> reader, such a check for transaction would make for a jarring
> experience; "why are we checking for active transaction in  the guts
> of guc.c?", they might think. If anything, such an error should be
> thrown from or below pg_parameter_aclcheck().
>
> But I am not sure if it should be exposed as an error. A user
> encountering that error is not at fault. Hence I believe an assertion
> check is more suitable for catching code that invokes
> set_config_option() outside a transaction.

Please see attached the patch that implements the above proposal.

The process_session_preload_libraries() call has been moved to the end
of InitPostgres(), just before we report the backend to
PgBackendStatus and commit the first transaction.

One notable side effect of this change is that
process_session_preload_libraries() is now called _before_ we
SetProcessingMode(NormalProcessing). Which means any database access
performed by _PG_init() of an extension will be doing it in
InitProcessing mode. I'm not sure if that's problematic.

The patch also adds an assertion in pg_parameter_aclcheck() to ensure
that there's a transaction is in progress before it's called.

The patch now lets the user connect, throws a warning, and does not crash.

$ PGOPTIONS="-c plperl.on_plperl_init=" psql -U test
WARNING:  permission denied to set parameter "plperl.on_plperl_init"
Expanded display is used automatically.
psql (15beta1)
Type "help" for help.

postgres@B:694512=>

Best regards,
Gurjeet
http://Gurje.et

move_process_session_preload_libraries.patch
Description: Binary data

Re: `make check` doesn't pass on MacOS Catalina

2022-08-06 Thread Gurjeet Singh

On Tue, Apr 20, 2021 at 9:06 AM Andrew Dunstan  wrote:
>
> On 4/20/21 11:02 AM, Tom Lane wrote:
> > Aleksander Alekseev  writes:
> >> While trying to build PostgreSQL from source (master branch, 95c3a195) on a
> >> MacBook I discovered that `make check` fails:
> > This is the usual symptom of not having disabled SIP :-(.
> >
> > If you don't want to do that, do "make install" before "make check".

> FYI the buildfarm client has a '--delay-check' option that does exactly
> this. It's useful on Alpine Linux as well as MacOS

I was trying to set up a buildfarm animal, and this exact problem lead
to a few hours of debugging and hair-pulling. Can the default
behaviour be changed in buildfarm client to perform `make check` only
after `make install`.

Current buildfarm client code looks something like:

make();
make_check() unless $delay_check;
... other steps ...
make_install();
... other steps-2...
make_check() if $delay_check;

There are no comments as to why one should choose to use --delay-check
($delay_check). This email, and the pointer to the paragraph buried in
the docs, shared by Tom, are the only two ways one can understand what
is causing this failure, and how to get around it.

Naive question: What's stopping us from rewriting the code as follows.
make();
make_install();
make_check();
... other steps ...
... other steps-2...
# or move make_check() call here

With a quick google search I could not find why --delay-check is
necessary on Apline linux, as well; can you please elaborate.

Best regards,
Gurjeet
http://Gurje.et

Re: Patch proposal: New hooks in the connection path

2022-08-13 Thread Gurjeet Singh

On Mon, Aug 8, 2022 at 3:51 AM Drouvot, Bertrand  wrote:
> Please find attached v2-0004-connection_hooks.patch

 /*
  * Stop here if it was bad or a cancel packet.  ProcessStartupPacket
  * already did any appropriate error reporting.
  */
 if (status != STATUS_OK)
+{
+#ifndef EXEC_BACKEND
+if (FailedConnection_hook)
+(*FailedConnection_hook) (FCET_BAD_STARTUP_PACKET, port);
+#endif
 proc_exit(0);
+}

Per the comment above the if condition, the `status != OK` may
represent a cancel packet, as well. Clearly, a cancel packet is not
the same as a _bad_ packet. So I think here you need to differentiate
between a cancel packet and a genuinely bad packet; I don't see
anything good coming good out of us, or the hook-developer, lumping
those 2 cases together.

I think we can reduce the number of places the hook is called, if we
call the hook from proc_exit(), and all the other places we simply set
a global variable to signify the reason for the failure. The case of
_exit(1) from the signal-handler cannot use such a mechanism, but I
think all the other cases of interest can simply register one of the
FCET_* value, and the hook call from proc_exit() can pass that value
to the hook.

If we can convinces ourselves that we can use proc_exit(1) in
StartupPacketTimeoutHandler(), instead of calling _exit(1), I think we
cal eliminate replace all call sites for this hook with
set-global-variable variant.

> ...
> * This should be the only function to call exit().
> * -cim 2/6/90
>...
> proc_exit(int code)

The comment on proc_exit() claims that should be the only place
calling exit(), except the add-on/extension hooks. So there must be a
strong reason why the signal-handler uses _exit() to bypass all
callbacks.

Best regards,
Gurjeet
http://Gurje.et

Re: Patch proposal: New hooks in the connection path

2022-08-13 Thread Gurjeet Singh

(reposting the same review, with many grammatical fixes)

On Mon, Aug 8, 2022 at 3:51 AM Drouvot, Bertrand  wrote:
> Please find attached v2-0004-connection_hooks.patch

 /*
  * Stop here if it was bad or a cancel packet.  ProcessStartupPacket
  * already did any appropriate error reporting.
  */
 if (status != STATUS_OK)
+{
+#ifndef EXEC_BACKEND
+if (FailedConnection_hook)
+(*FailedConnection_hook) (FCET_BAD_STARTUP_PACKET, port);
+#endif
 proc_exit(0);
+}

Per the comment above the if condition, the `status != OK` may
represent a cancel packet, as well. Clearly, a cancel packet is not
the same as a _bad_ packet. So I think here you need to differentiate
between a cancel packet and a genuinely bad packet; I don't see
anything good coming out of us, or the hook-developer, lumping
those 2 cases together.

I think we can reduce the number of places the hook is called, if we
call the hook from proc_exit(), and at all the other places we simply set
a global variable to signify the reason for the failure. The case of
_exit(1) from the signal-handler cannot use such a mechanism, but I
think all the other cases of interest can simply register one of the
FCET_* values, and let the call from proc_exit() pass that value
to the hook.

If we can convince ourselves that we can use proc_exit(1) in
StartupPacketTimeoutHandler(), instead of calling _exit(1), I think we
cal replace all call sites for this hook with the
set-global-variable variant.

> ...
> * This should be the only function to call exit().
> * -cim 2/6/90
>...
> proc_exit(int code)

The comment on proc_exit() claims that it should be the only place
calling exit(), except that the add-on/extension hooks may ignore this.
So there must be a strong reason why the signal-handler uses _exit()
to bypass all callbacks.

Best regards,
Gurjeet
http://Gurje.et

Returning to Postgres community work

2021-08-30 Thread Gurjeet Singh

Hi All,

 I'm very happy to announce that I now work for Supabase [1]. They
have hired me so that I can participate in, and contribute to the
Postgres community.

I'm announcing it here in the hopes that more companies feel
encouraged to contribute to Postgres. For those who don't know my past
work and involvement in the Postgres community, please see the
'PostgreSQL RDBMS' section in my resume [2] (on page 4).

I'm deeply indebted to Supabase for giving me this opportunity to
work with, and for the Postgres community.

Following is the statement by Paul (CEO) and Anthony (CTO), the
co-founders of Supabase:

Supabase is a PostgreSQL hosting service that makes PostgreSQL
incredibly easy to use. Since our inception in 2020 we've benefited
hugely from the work of the PostgreSQL community.

We've been long-time advocates of PostgreSQL, and we're now in a
position to contribute back in a tangible way. We're hiring Gurjeet
with the explicit goal of working on PostgreSQL community
contributions. We're excited to welcome Gurjeet to the team at
Supabase.

[1]: https://supabase.io/
[2]: https://gurjeet.singh.im/GurjeetResume.pdf

PS: Hacker News announcement is at https://news.ycombinator.com/item?id=

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

Re: Returning to Postgres community work

2021-08-30 Thread Gurjeet Singh

On Mon, Aug 30, 2021 at 10:53 PM Gurjeet Singh  wrote:

> PS: Hacker News announcement is at https://news.ycombinator.com/item?id=

https://news.ycombinator.com/item?id=28364406

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

Re: Returning to Postgres community work

2021-08-31 Thread Gurjeet Singh

On Tue, Aug 31, 2021 at 8:04 AM Alvaro Herrera  wrote:
>
> On 2021-Aug-30, Gurjeet Singh wrote:
>
> >  I'm very happy to announce that I now work for Supabase [1]. They
> > have hired me so that I can participate in, and contribute to the
> > Postgres community.
>
> Hey Gurjeet, welcome back.  Glad to hear you've found a good spot.

Thank you!

> You know what I've heard?  That your index advisor module languishes
> unmaintained and that there's no shortage of people wishing to use it.

Now there's a masterclass in making someone feel great and guilty in
the same sentence ;-)

> Heck, we spent a lot of back-and-forth in the spanish mailing list
> with somebody building a super obsolete version of Postgres just so that
> they could compile your index advisor.  I dunno, if you have some spare
> time, maybe updating that one would be a valuable contribution from
> users' perspective.

Aye-aye Capn' :-)

EDB folks reached out to me a few months ago to assign a license to
the project, which I did and it is now a Postgres-licensed project
[1].

Given the above, it is safe to assume that this tool is at least being
maintained by EDB, at least internally for their customers. I would
request them to contribute the changes back in the open.

Regardless of that, please rest assured that I will work on making it
compatible with the current supported versions of Postgres. Lack of
time is not an excuse anymore :-)

Thanks for bringing this to my attention!

[1]: https://github.com/gurjeet/pg_adviser/blob/master/LICENSE

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

Re: Slightly improve initdb --sync-only option's help message

2021-07-28 Thread Gurjeet Singh

On Mon, Jul 26, 2021 at 11:05 AM Bossart, Nathan  wrote:
> Here are my suggestions in patch form.

+printf(_("  -S, --sync-only   safely write all database
files to disk and exit\n"));

Not your patch's fault, but the word "write" does not seem to convey
the true intent of the option, because generally a "write" operation
is still limited to dirtying the OS buffers, and does not guarantee
sync-to-disk.

It'd be better if the help message said, either "flush all database
files to disk and exit",or "sync all database files to disk and exit".

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

Re: Slightly improve initdb --sync-only option's help message

2021-08-16 Thread Gurjeet Singh

On Mon, Aug 16, 2021 at 4:42 AM Daniel Gustafsson  wrote:
>
> > On 30 Jul 2021, at 18:27, Bossart, Nathan  wrote:
> >
> > On 7/30/21, 2:22 AM, "Daniel Gustafsson"  wrote:
> >> LGTM.  I took the liberty to rephrase the "and exit" part of the initdb 
> >> help
> >> output match the other exiting options in there.  Barring objections, I 
> >> think
> >> this is ready.
> >
> > LGTM.  Thanks!
>
> Pushed to master, thanks!

Thank you Daniel and Nathan! Much appreciated.

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

Proposal: Division operator for (interval / interval => double precision)

2024-06-23 Thread Gurjeet Singh

Is there a desire to have a division operator / that takes dividend
and divisor of types interval, and results in a quotient of type
double precision.

This would be helpful in calculating how many times the divisor
interval can fit into the dividend interval.

To complement this division operator, it would be desirable to also
have a remainder operator %.

For example,

('365 days'::interval / '5 days'::interval) => 73
('365 days'::interval % '5 days'::interval) => 0

('365 days'::interval / '3 days'::interval) => 121
('365 days'::interval % '3 days'::interval) => 2

Best regards,
Gurjeet
http://Gurje.et

Re: DecodeInterval fixes

2023-07-08 Thread Gurjeet Singh

On Sat, Jul 8, 2023 at 1:33 PM Tom Lane  wrote:
>
> Gurjeet Singh  writes:
> > On Fri, Jul 7, 2023 at 4:13 PM Tom Lane  wrote:
> >> The ECPG datetime datatype support code has been basically unmaintained
> >> for years, and has diverged quite far at this point from the core code.
>
> > I was under the impression that anything in the postgresql.git
> > repository is considered core, and hence maintained as one unit, from
> > release to release.
>
> When I say that ecpglib is next door to unmaintained, I'm just stating
> the facts on the ground; project policy is irrelevant.  That situation
> is not likely to change until somebody steps up to do (a lot of) work
> on it, which is probably unlikely to happen unless we start getting
> actual complaints from ECPG users.  For the meantime, what's there now
> seems to be satisfactory to whoever is using it ... which might be
> nobody?
>
> In any case, you don't have to look far to notice that some parts of
> the tree are maintained far more actively than others.  ecpglib is
> just one of the more identifiable bits that's receiving little love.
> The quality of the code under contrib/ is wildly variable, and even
> the server code itself has backwaters.  (For instance, the "bit" types,
> which aren't even in the standard anymore; or the geometric types,
> or "money".)

Thanks for sharing your view on different parts of the code. This give
a clear direction if someone would be interested in stepping up.

As part of my mentoring a GSoC 2023 participant, last night we were
looking at the TODO wiki page, for something for the mentee to pick up
next. I feel the staleness/deficiencies you mention above are not
captured in the TODO wiki page. It'd be nice if these were documented,
so that newcomers to the community can pick up work that they feel is
an easy lift for them.

> By and large, I don't see this unevenness of maintenance effort as
> a problem.  It's more like directing our limited resources into the
> most useful places.  Code that isn't getting worked on is either not
> used at all by anybody, or it serves the needs of those who use it
> well enough already.  Since it's difficult to tell which of those
> cases applies, removing code just because it's not been improved
> lately is a hard choice to sell.  But so is putting maintenance effort
> into code that there might be little audience for.  In the end we
> solve this via the principle of "scratch your own itch": if somebody
> is concerned enough about a particular piece of code to put their own
> time into improving it, then great, it'll get improved.
>
> > Benign neglect doesn't sound nice from a user's/consumer's
> > perspective. Can it be labeled (i.e. declared as such in docs) as
> > deprecated.
>
> Deprecation would imply that we're planning to remove it, which
> we are not.

Good to know. Sorry I took "benign neglect" to mean that there's no
willingness to improve it. This makes it clear that community wants to
improve and maintain ECPG, it's just a matter of someone volunteering,
and better use of the resources available.



Best regards,
Gurjeet
http://Gurje.et

Re: BUG #18016: REINDEX TABLE failure

2023-07-10 Thread Gurjeet Singh

On Sun, Jul 9, 2023 at 7:21 AM Richard Veselý  wrote:
>
> ... there's no shortage of people that suffer from sluggish 
> pg_dump/pg_restore cycle and I imagine there are any number of people that 
> would be interested in improving bulk ingestion which is often a bottleneck 
> for analytical workloads as you are well aware. What's the best place to 
> discuss this topic further - pgsql-performance or someplace else?

(moved conversation to -hackers, and moved -bugs to BCC)

> I was dissatisfied with storage layer performance, especially during the 
> initial database population, so I rewrote it for my use case. I'm done with 
> the heap, but for the moment I still rely on PostgreSQL to build indexes,

It sounds like you've developed a method to speed up loading of
tables, and might have ideas/suggestions for speeding up CREATE
INDEX/REINDEX. The -hackers list feels like a place to discuss such
changes.

Best regards,
Gurjeet
http://Gurje.et

Re: BUG #18016: REINDEX TABLE failure

On Sun, Jul 9, 2023 at 7:18 AM Tom Lane  wrote:
>
> Michael Paquier  writes:
> > That should be OK, I assume.  However, if this is improved and
> > something we want to support in the long-run I guess that a TAP test
> > may be appropriate.
>
> I do not see the point of a TAP test.  It's not like the code isn't
> covered perfectly well.

Please find attached the patch that makes REINDEX TABLE perform
reindex on toast table before reindexing the main table's indexes.

The code block movement involved slightly more thought and care than I
had previously imagined. As explained in comments in the patch, the
enumeration and suppression of indexes on the main table must happen
before any CommandCounterIncrement() call, hence the
reindex-the-toast-table-if-any code had to be placed after that
enumeration.

In support of the argument above, the patch does not include any TAP
tests. Reliably reproducing the original error message involves
restarting the database, and since that can't be done via SQL
commands, no sql tests are included, either.

The patch also includes minor wordsmithing, and benign whitespace
changes in neighboring code.

Best regards,
Gurjeet
http://Gurje.et

v1-0001-Reindex-toast-table-s-index-before-main-table-s-i.patch
Description: Binary data

Re: Document that server will start even if it's unable to open some TCP/IP ports

On Mon, Jun 19, 2023 at 5:48 PM Bruce Momjian  wrote:
>
> On Tue, Jun 13, 2023 at 11:11:04PM -0400, Tom Lane wrote:
> >
> > There is certainly an argument that such a condition indicates that
> > something's very broken in our configuration and we should complain.
> > But I'm not sure how exciting the case is in practice.  The systemd
> > guys would really like us to be willing to come up before any network
> > interfaces are up, and then auto-listen to those interfaces when they
> > do come up.

That sounds like a reasonable expectation, as the network conditions
can change without any explicit changes made by someone.

> > On the other hand, the situation with Unix sockets is
> > much more static: if you can't make a socket in /tmp or /var/run at
> > the instant of postmaster start, it's unlikely you will be able to do
> > so later.

I think you're describing a setup where Postgres startup is automatic,
as part of server/OS startup. That is the most common case.

In cases where someone is performing a Postgres startup manually, they
are very likely to have the permissions to fix the problem preventing
the startup.

> > Maybe we need different rules for TCP versus Unix-domain sockets?
> > I'm not sure what exactly, but lumping those cases together for
> > a discussion like this feels wrong.

+1.

> If we are going to retry for network configuration changes, it seems we
> would also retry Unix domain sockets for cases like when the permissions
> are wrong, and then fixed.

The network managers (systemd, etc.) are expected to respond to
dynamic conditions, and hence they may perform network config changes
in response to things like network outages, and hardware failures,
time of day, etc.

On the other hand, the permissions required to create files for Unix
domain sockets are only expected to change if someone decides to make
that change. I wouldn't expect these permissions to be changed
dynamically.

On those grounds, keeping the treatment of Unix domain sockets out of
this discussion for this patch seems reasonable.

> However, it seem hard to figure out exactly what _is_ working if we take
> the approach of dynamically retrying listen methods.  Do we report
> anything helpful in the server logs when we start and can't listen on
> anything?

Yes. For every host listed in listen_addresses, if Postgres fails to
open the port on that address, we get a WARNING message in the server
log. After the end of processing of a non-empty listen_addresses, if
there are zero open TCP/IP connections, the server exits (with a FATAL
message, IIRC).

Best regards,
Gurjeet
http://Gurje.et

Re: MERGE ... RETURNING

On Fri, Jul 7, 2023 at 3:48 PM Dean Rasheed  wrote:
>
> On Thu, 6 Jul 2023 at 06:13, Gurjeet Singh  wrote:
> >
> > > One minor annoyance is that, due to the way that pg_merge_action() and
> > > pg_merge_when_clause() require access to the MergeActionState, they
> > > only work if they appear directly in the RETURNING list. They can't,
> > > for example, appear in a subquery in the RETURNING list, and I don't
> > > see an easy way round that limitation.
> >
> > I believe that's a serious limitation, and would be a blocker for the 
> > feature.
>
> Yes, that was bugging me for quite a while.
>
> Attached is a new version that removes that restriction, allowing the
> merge support functions to appear anywhere. This requires a bit of
> planner support, to deal with merge support functions in subqueries,
> in a similar way to how aggregates and GROUPING() expressions are
> handled. But a number of the changes from the previous patch are no
> longer necessary, so overall, this version of the patch is slightly
> smaller.

+1

> > I think the name of function pg_merge_when_clause() can be improved.
> > How about pg_merge_when_clause_ordinal().
> >
>
> That's a bit of a mouthful, but I don't have a better idea right now.
> I've left the names alone for now, in case something better occurs to
> anyone.

+1. How do we make sure we don't forget that it needs to be named
better. Perhaps a TODO item within the patch will help.

> > In the doc page of MERGE, you've moved the 'with_query' from the
> > bottom of Parameters section to the top of that section. Any reason
> > for this? Since the Parameters section is quite large, for a moment I
> > thought the patch was _adding_ the description of 'with_query'.
> >
>
> Ah yes, I was just making the order consistent with the
> INSERT/UPDATE/DELETE pages. That could probably be committed
> separately.

I don't think that's necessary, if it improves consistency with related docs.

> > +/*
> > + * Merge support functions should only be called directly from a MERGE
> > + * command, and need access to the parent ModifyTableState.  The parser
> > + * should have checked that such functions only appear in the RETURNING
> > + * list of a MERGE, so this should never fail.
> > + */
> > +if (IsMergeSupportFunction(funcid))
> > +{
> > +if (!state->parent ||
> > +!IsA(state->parent, ModifyTableState) ||
> > +((ModifyTableState *) state->parent)->operation != CMD_MERGE)
> > +elog(ERROR, "merge support function called in non-merge 
> > context");
> >
> > As the comment says, this is an unexpected condition, and should have
> > been caught and reported by the parser. So I think it'd be better to
> > use an Assert() here. Moreover, there's ERROR being raised by the
> > pg_merge_action() and pg_merge_when_clause() functions, when they
> > detect the function context is not appropriate.
> >
> > I found it very innovative to allow these functions to be called only
> > in a certain part of certain SQL command. I don't think  there's a
> > precedence for this in  Postgres; I'd be glad to learn if there are
> > other functions that Postgres exposes this way.
> >
> > -EXPR_KIND_RETURNING,/* RETURNING */
> > +EXPR_KIND_RETURNING,/* RETURNING in INSERT/UPDATE/DELETE */
> > +EXPR_KIND_MERGE_RETURNING,  /* RETURNING in MERGE */
> >
> > Having to invent a whole new ParseExprKind enum, feels like a bit of
> > an overkill. My only objection is that this is exactly like
> > EXPR_KIND_RETURNING, hence EXPR_KIND_MERGE_RETURNING needs to be dealt
> > with exactly in as many places. But I don't have any alternative
> > proposals.
> >
>
> That's all gone now from the new patch, since there is no longer any
> restriction on where these functions can appear.

I believe this elog can be safely turned into an Assert.

+switch (mergeActionCmdType)
 {
-CmdType commandType = relaction->mas_action->commandType;

+case CMD_INSERT:

+default:
+elog(ERROR, "unrecognized commandType: %d", (int)
mergeActionCmdType);

The same treatment can be applied to the elog(ERROR) in pg_merge_action().

> > +CREATE FUNCTION merge_into_sq_target(sid int, balance int, delta int,
> > + OUT action text, OUT tid int,
> > OUT new_balance int)
> > +LANGUAGE sql AS
> > +$$
> > +MERGE

Re: MERGE ... RETURNING

On Tue, Jul 11, 2023 at 1:43 PM Gurjeet Singh  wrote:
>
> In the above hunk, if there's an exception/ERROR, I believe we should
> PG_RE_THROW(). If there's a reason to continue, we should at least set
> rslot = NULL, otherwise we may be returning an uninitialized value to
> the caller.

Excuse the brain-fart on my part. There's not need to PG_RE_THROW(),
since there's no PG_CATCH(). Re-learning the code's infrastructure
slowly :-)

Best regards,
Gurjeet
http://Gurje.et

Better help output for pgbench -I init_steps

These patches were created during an unrelated discussion about pgbench.

Please see emails [1] - [6] linked below, for the past discussion.

In brief:

> $ pgbench -i -I dtGvp -s 500

The init-steps are severely under-documented in pgbench --help output.
I think at least a pointer to the the pgbench docs should be mentioned
in the pgbench --help output; an average user may not rush to read the
code to find the explanation, but a hint to where to find more details
about what the letters in --init-steps mean, would save them a lot of
time.

Please see attached 4 variants of the patch. Variant 1 simply tells
the reader to consult pgbench documentation. The second variant
provides a description for each of the letters, as the documentation
does. The committer can pick the one they find suitable.

The text ", in the specified order" is an important detail, that
should be included irrespective of the rest of the patch.

My preference would be to use the first variant, since the second one
feels too wordy for --help output. I believe we'll have to choose
between these two; the alternatives will not make anyone happy.

These two variants show the two extremes; bare minimum vs. everything
but the kitchen sink. So one may feel the need to find a middle ground
and provide a "sufficient, but not too much" variant. I have attempted
that in variants 3 and 4; also attached.

The third variant does away with the list of steps, and uses a
paragraph to describe the letters. And the fourth variant makes that
paragraph terse.

In the order of preference I'd choose variant 1, then 2. Variants 3
and 4 feel like a significant degradation from variant 2.

Attached samples.txt shows the snippets of --help output of each of
the variants/patches, for comparison.

In [6] below, Tristan showed preference for the second variant.

[1] My complaint about -I initi_steps being severely under-documented
in --help output
https://www.postgresql.org/message-id/CABwTF4XMdHTxemhskad41Vj_hp2nPgifjwegOqR52_8-wEbv2Q%40mail.gmail.com

[2] Tristan Partin agreeing with the complaint, and suggesting a patch
would be welcome
https://www.postgresql.org/message-id/CT8BC7RXT33R.3CHYIXGD5NVHK%40gonk

[3] Greg Smith agreeing and saying he'd welcome a few more words about
the init_steps in --help output
https://www.postgresql.org/message-id/CAHLJuCUp5_VUo%2BRJ%2BpSnxeiiZfcstRtTubRP8%2Bu8NEqmrbp4aw%40mail.gmail.com

[4] First set of patches
https://www.postgresql.org/message-id/CABwTF4UKv43ZftJadsxs8%3Da07BmA1U4RU3W1qbmDAguVKJAmZw%40mail.gmail.com

[5] Second set of patches
https://www.postgresql.org/message-id/CABwTF4Ww42arY1Q88_iaraVLxqSU%2B8Yb4oKiTT5gD1sineog9w%40mail.gmail.com

[6] Tristan showing preference for the second variant
https://www.postgresql.org/message-id/CTBN5E2K2YSJ.3QYXGZ09JZXIW%40gonk

+CC Tristan Partin and Greg Smith, since they were involved in the
initial thread.

Best regards,
Gurjeet
http://Gurje.et
variant-1-brief-pointer-to-docs.patch
-i, --initialize invokes initialization mode
-I, --init-steps=[dtgGvpf]+ (default "dtgvp")
run selected initialization steps, in the specified
order
see pgbench documentation for a description of these
steps
-F, --fillfactor=NUM set fill factor

variant-2-detailed-description.patch
-i, --initialize invokes initialization mode
-I, --init-steps=[dtgGvpf]+ (default "dtgvp")
run selected initialization steps, in the specified
order
d: Drop any existing pgbench tables
t: Create the tables used by the standard pgbench
scenario
g: Generate data, client-side
G: Generate data, server-side
v: Invoke VACUUM on the standard tables
p: Create primary key indexes on the standard tables
f: Create foreign key constraints between the
standard tables
-F, --fillfactor=NUM set fill factor

variant-3-details-not-list.patch
-i, --initialize invokes initialization mode
-I, --init-steps=[dtgGvpf]+ (default "dtgvp")
run selected initialization steps, in the specified
order.
These steps operate on standard tables used by
pgbench
'd' drops the tables, 't' creates the tables, 'g'
generates
the data on client-side, 'G' generates data on
server-side,
'v' vaccums the tables, 'p' creates primary key
constraints,
and 'f' creates foreign key constraints
-F, --fillfactor=NUM set fill factor

variant-4-terse-details-not-list.patch
-i, --initialize invokes initialization

Re: Better help output for pgbench -I init_steps

On Wed, Jul 12, 2023 at 3:08 AM Peter Eisentraut  wrote:
>
> On 12.07.23 09:42, Gurjeet Singh wrote:
> > These two variants show the two extremes; bare minimum vs. everything
> > but the kitchen sink. So one may feel the need to find a middle ground
> > and provide a "sufficient, but not too much" variant. I have attempted
> > that in variants 3 and 4; also attached.
>
> If you end up with variant 3 or 4, please use double quotes instead of
> single quotes.

Will  do.

I believe you're suggesting this because in the neighboring help text
the string literals use double quotes. I see that other utilities,
such as psql also use double quotes in help  text.

If there's a convention, documented somewhere in our sources, I'd love
to know and learn other conventions.

Best regards,
Gurjeet
http://Gurje.et

Re: CommandStatus from insert returning when using a portal.

On Wed, Jul 12, 2023 at 1:03 PM Dave Cramer  wrote:
>
> With a simple insert such as
>
> INSERT INTO test_table (cnt) VALUES (1), (2) RETURNING id
>
> if a portal is used to get the results then the CommandStatus is not returned 
> on the execute only when the portal is closed. After looking at this more it 
> is really after all of the data is read which is consistent if you don't use 
> a portal, however it would be much more useful if we received the 
> CommandStatus after the insert was completed and before the data
>
> Obviously I am biased by the JDBC API which would like to have
>
> PreparedStatement.execute() return the number of rows inserted without having 
> to wait to read all of the rows returned

I believe if RETURNING clause is use, the protocol-level behaviour of
INSERT is expected to match that of SELECT. If the SELECT command
behaves like that (resultset followed by CommandStatus), then I'd say
the INSERT RETURNING is behaving as expected.

Best regards,
Gurjeet
http://Gurje.et

Re: Duplicated LLVMJitHandle->lljit assignment?

On Wed, Jul 12, 2023 at 5:22 PM Matheus Alcantara  wrote:
>
> I was reading the jit implementation and I notice that the lljit field of
> LLVMJitHandle is being assigned twice on llvm_compile_module function, is this
> correct? I'm attaching a supposed fixes that removes  the second assignment. I
> ran meson test and all tests have pass.

-handle->lljit = compile_orc;

LGTM.

Best regards,
Gurjeet
http://Gurje.et

Re: MERGE ... RETURNING

2023-07-13 Thread Gurjeet Singh

On Thu, Jul 13, 2023 at 8:38 AM Dean Rasheed  wrote:
>
> On Tue, 11 Jul 2023 at 21:43, Gurjeet Singh  wrote:

> >  { oid => '9499', descr => 'command type of current MERGE action',
> > -  proname => 'pg_merge_action',  provolatile => 'v',
> > +  proname => 'pg_merge_action',  provolatile => 'v', proparallel => 'r',
> > 
> >  { oid => '9500', descr => 'index of current MERGE WHEN clause',
> > -  proname => 'pg_merge_when_clause',  provolatile => 'v',
> > +  proname => 'pg_merge_when_clause',  provolatile => 'v', proparallel => 
> > 'r',
> > 
> >
> > I see that you've now set proparallel of these functions to 'r'. I'd
> > just like to understand how you got to that conclusion.
> >
>
> Now that these functions can appear in subqueries in the RETURNING
> list, there exists the theoretical possibility that the subquery might
> use a parallel plan (actually that can't happen today, for any query
> that modifies data, but maybe someday it may become a possibility),
> and it's possible to use these functions in a SELECT query inside a
> PL/pgSQL function called from the RETURNING list, which might consider
> a parallel plan. Since these functions rely on access to executor
> state that isn't copied to parallel workers, they must be run on the
> leader, hence I think PARALLEL RESTRICTED is the right level to use. A
> similar example is pg_trigger_depth().

Thanks for the explanation. That helps.

Best regards,
Gurjeet
http://Gurje.et

Re: Fix search_path for all maintenance commands

2023-07-13 Thread Gurjeet Singh

On Thu, Jul 13, 2023 at 11:56 AM Jeff Davis  wrote:
>
> The current approach seemed to get support from Noah, Nathan, and Greg
> Stark.
>
> Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but

I didn't see Tom's or Robert's concerns raised in this thread. I see
now that for some reason there are two threads with slightly different
subjects. I'll catch-up on that, as well.

The other thread's subject: "pgsql: Fix search_path to a safe value
during maintenance operations"

> I'm not sure whether those objections were specific to the 16 cycle or
> whether they are objections to the approach entirely. Comments?

The approach seems good to me. My concern is with this change's
potential to cause an extended database outage. Hence sending it out
as part of v16, without any escape hatch for the DBA, is my objection.

It it were some commercial database, we would have simply introduced a
hidden event, or a GUC, with default off. But a GUC for this feels too
heavy handed.

Perhaps we can provide an escape hatch as follows (warning, pseudocode ahead).

> if (first_element(search_path) != 
> "dont_override_search_patch_for_maintenance")
>  SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, ...);

So, if someone desperately wants to just plow through the maintenance
command, and are not ready or able to fix their dependence on their
search_path, the community can show them this escape-hatch.

Best regards,
Gurjeet
http://Gurje.et

Re: Fix search_path for all maintenance commands

2023-07-13 Thread Gurjeet Singh

On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston
 wrote:
>
>  I'm against simply breaking the past without any recourse as what we did for 
> pg_dump/pg_restore still bothers me.

I'm sure this is tangential, but can you please provide some
context/links to the change you're referring to here.

Best regards,
Gurjeet
http://Gurje.et

Re: harmonize password reuse in vacuumdb, clusterdb, and reindexdb

2023-07-18 Thread Gurjeet Singh

On Mon, Jul 17, 2023 at 1:47 PM Nathan Bossart  wrote:
>
> Here is a new version of the patch in which I've updated this comment as
> proposed.  Gurjeet, do you have any other concerns about this patch?

With the updated comment, the patch looks good to me.

Best regards,
Gurjeet
http://Gurje.et

Re: There should be a way to use the force flag when restoring databases

2023-07-19 Thread Gurjeet Singh

On Tue, Jul 18, 2023 at 12:53 AM Joan  wrote:
>
> Since posgres 13 there's the option to do a FORCE when dropping a database 
> (so it disconnects current users) Documentation here: 
> https://www.postgresql.org/docs/current/sql-dropdatabase.html
>
> I am currently using dir format for the output
>pg_dump -d "bdname" -F d -j 4 -v -f /tmp/dir
>
> And restoring the database with
>   pg_restore -d postgres -C -c --exit-on-error -F d -j 3 -v /tmp/dir
>
> Having an option to add the FORCE option to either the generated dump by 
> pg_dump, or in the pg_restore would be very useful when restoring the 
> databases to another servers so it would avoid having to do scripting.
>
> In my specific case I am using this to refresh periodically a development 
> environment with data from production servers for a small database (~200M).

Making force-drop a part of pg_dump output may be dangerous, and not
provide much flexibility at restore time.

Adding a force option to pg_restore feels like providing the right tradeoff.

The docs for 'pg_restore --create` say "Create the database before
restoring into it. If --clean is also specified, drop and recreate the
target database before connecting to it."

If we provided a force option, it may then additionally say: "If the
--clean and --force options are specified, DROP DATABASE ... WITH
FORCE command will be used to drop the database."

Using WITH FORCE is not a guarantee, as the DROP DATABASE docs clarify
the conditions under which it may fail.

Best regards,
Gurjeet
http://Gurje.et

Re: Issue in _bt_getrootheight

2023-07-19 Thread Gurjeet Singh

On Tue, Jul 11, 2023 at 9:35 AM Ahmed Ibrahim
 wrote:
>
> We have been working on the pg_adviser extension whose goal is to suggest 
> indexes by creating virtual/hypothetical indexes and see how it affects the 
> query cost.
>
> The hypothetical index shouldn't take any space on the disk (allocates 0 
> pages) so we give it the flag INDEX_CREATE_SKIP_BUILD.
> But the problem comes from here when the function get_relation_info is called 
> in planning stage, it tries to calculate the B-Tree height by calling 
> function _bt_getrootheight, but the B-Tree is not built at all, and its 
> metadata page (which is block 0 in our case) doesn't exist, so this returns 
> error that it cannot read the page (since it doesn't exist).
>
> I tried to debug the code and found that this feature was introduced in 
> version 9.3 under this commit [1]. I think that in the code we need to check 
> if it's a B-Tree index AND the index is built/have some pages, then we can go 
> and calculate it otherwise just put it to -1

> I mean instead of this
> if (info->relam == BTREE_AM_OID)
> {
> /* For btrees, get tree height while we have the index open */
> info->tree_height = _bt_getrootheight(indexRelation);
> }
> else
> {
>  /* For other index types, just set it to "unknown" for now */
> info->tree_height = -1;
> }
>
> The first line should be
> if (info->relam == BTREE_AM_OID && info->pages > 0)
> or use the storage manager (smgr) to know if the first block exists.

I think the better method would be to calculate the index height
*after* get_relation_info_hook is called. That way, instead of the
server guessing whether or not an index is hypothetical it can rely on
the index adviser's notion of which index is hypothetical. The hook
implementer has the opportunity to not only mark the
indexOptInfo->hypothetical = true, but also calculate the tree height,
if they can.

Please see attached the patch that does this. Let me know if this patch helps.

Best regards,
Gurjeet
http://Gurje.et


calculate-index-height-after-calling-get_relation_info_hook.patch
Description: Binary data

Re: There should be a way to use the force flag when restoring databases

On Thu, Jul 20, 2023 at 2:10 AM Daniel Gustafsson  wrote:
>
> > On 19 Jul 2023, at 19:28, Gurjeet Singh  wrote:
> >
> > The docs for 'pg_restore --create` say "Create the database before
> > restoring into it. If --clean is also specified, drop and recreate the
> > target database before connecting to it."
> >
> > If we provided a force option, it may then additionally say: "If the
> > --clean and --force options are specified, DROP DATABASE ... WITH
> > FORCE command will be used to drop the database."
>
> pg_restore --clean refers to dropping any pre-existing database objects and 
> not
> just databases, but --force would only apply to databases.
>
> I wonder if it's worth complicating pg_restore with that when running dropdb
> --force before pg_restore is an option for those wanting to use WITH FORCE.

Fair point. But the same argument could've been applied to --clean
option, as well; why overload the meaning of --clean and make it drop
database, when a dropdb before pg_restore was an option.

IMHO, if pg_restore offers to drop database, providing an option to
the user to do it forcibly is not that much of a stretch, and within
reason for the user to expect it to be there, like Joan did.

Best regards,
Gurjeet
http://Gurje.et

Re: MERGE ... RETURNING

On Fri, Jul 14, 2023 at 1:55 AM Dean Rasheed  wrote:
>
> On Thu, 13 Jul 2023 at 20:14, Jeff Davis  wrote:
> >
> > On Thu, 2023-07-13 at 18:01 +0100, Dean Rasheed wrote:
> > > For some use cases, I can imagine allowing OLD/NEW.colname would mean
> > > you wouldn't need pg_merge_action() (if the column was NOT NULL), so
> > > I
> > > think the features should work well together.
> >
> > For use cases where a user could do it either way, which would you
> > expect to be the "typical" way (assuming we supported the new/old)?
> >
> >   MERGE ... RETURNING pg_merge_action(), id, val;
> >
> > or
> >
> >   MERGE ... RETURNING id, OLD.val, NEW.val;
> >
> > ?
> >
>
> I think it might depend on whether OLD.val and NEW.val were actually
> required, but I think I would still probably use pg_merge_action() to
> get the action, since it doesn't rely on specific table columns being
> NOT NULL.

+1. It would be better to expose the action explicitly, rather than
asking the user to deduce it based on the old and new values of a
column. The server providing that value is better than letting users
rely on error-prone methods.

> I found a 10-year-old thread discussing adding support for OLD/NEW to
> RETURNING [1],

Thanks for digging up that thread. An important concern brought up in
that thread was how the use of names OLD and NEW will affect plpgsql
(an possibly other PLs) trigger functions, which rely on specific
meaning for those names. The names BEFORE and AFTER, proposed there
are not as intuitive as OLD/NEW for the purpose of identifying old and
new versions of the row, but I don't have a better proposal. Perhaps
PREVIOUS and CURRENT?

> but it doesn't look like anything close to a
> committable solution was developed, or even a design that might lead
> to one. That's a shame, because there seemed to be a lot of demand for
> the feature,

+1

> > I am still bothered that pg_merge_action() is so context-sensitive.
> > "SELECT pg_merge_action()" by itself doesn't make any sense, but it's
> > allowed in the v8 patch. We could make that a runtime error, which
> > would be better, but it feels like it's structurally wrong. This is not
> > an objection, but it's just making me think harder about alternatives.
> >
> > Maybe instead of a function it could be a special table reference like:
> >
> >   MERGE ... RETURNING MERGE.action, MERGE.action_number, id, val?

I believe Jeff meant s/action_number/when_number/. Not that we've
settled on a name for this virtual column.

> Well, that's a little more concise, but I'm not sure that it really
> buys us that much, to be worth the extra complication.

After considering the options, and their pros and cons (ease of
implementation, possibility of conflict with SQL spec, intuitiveness
of syntax), I'm now strongly leaning towards the SQL syntax variant.
Exposing the action taken via a context-sensitive function feels
kludgy, when compared to Jeff's proposed SQL syntax. Don't get me
wrong, I still feel it was very clever how you were able to make the
function context sensitive, and make it work in expressions deeper in
the subqueries.

Plus, if we were able to make it work as SQL syntax, it's very likely
we can use the same technique to implement BEFORE and AFTER behaviour
in UPDATE ... RETURNING that the old thread could not accomplish a
decade ago.

> Presumably
> something in the planner would turn that into something the executor
> could handle, which might just end up being the existing functions
> anyway.

If the current patch's functions can serve the needs of the SQL syntax
variant, that'd be a neat win!

Best regards,
Gurjeet
http://Gurje.et

Re: MERGE ... RETURNING

On Mon, Jul 17, 2023 at 12:43 PM Jeff Davis  wrote:
>
> On Fri, 2023-07-14 at 09:55 +0100, Dean Rasheed wrote:
> > I found a 10-year-old thread discussing adding support for OLD/NEW to
> > RETURNING [1], but it doesn't look like anything close to a
> > committable solution was developed, or even a design that might lead
> > to one. That's a shame, because there seemed to be a lot of demand
> > for
> > the feature, but it's not clear how much effort it would be to
> > implement.
>
> It looks like progress was made in the direction of using a table alias
> with executor support to bring the right attributes along.

That patch introduced RTE_ALIAS to carry that info through to the
executor, and having to special-case that that in many places was seen
as a bad thing.

> There was some concern about how exactly the table alias should work
> such that it doesn't look too much like a join. Not sure how much of a
> problem that is.

My understanding of that thread is that the join example Robert shared
was for illustrative purposes only, to show that executor already has
enough information to produce the desired output, and to show that
it's a matter of tweaking the parser and planner to tell the executor
what output to produce. But later reviewers pointed out that it's not
that simple (example was given of ExecDelete performing
pullups/working hard to get the correct values of the old version of
the row).

The concerns were mainly around use of OLD.* and NEW.*, too much
special-casing of RTE_ALIAS, and then the code quality of the patch
itself.

> > > Maybe instead of a function it could be a special table reference
> > > like:
> > >
> > >   MERGE ... RETURNING MERGE.action, MERGE.action_number, id, val?
> > >
> >
> > Well, that's a little more concise, but I'm not sure that it really
> > buys us that much, to be worth the extra complication. Presumably
> > something in the planner would turn that into something the executor
> > could handle, which might just end up being the existing functions
> > anyway.
>
> The benefits are:
>
> 1. It is naturally constrained to the right context.

+1

> I'm not sure how much extra complication it would cause, though.

If that attempt with UPDATE RETURNING a decade ago is any indication,
it's probably a tough one.

Best regards,
Gurjeet
http://Gurje.et

Re: table_open/table_close with different lock mode