Re: Overhauling "Routine Vacuuming" docs, particularly its handling of freezing

[samay sharma]

Hi,

On Wed, May 3, 2023 at 3:48 PM Peter Geoghegan  wrote:

> On Wed, May 3, 2023 at 2:59 PM Peter Geoghegan  wrote:
> > Coming up with a new user-facing name for xidStopLimit is already on
> > my TODO list (it's surprisingly hard). I have used that name so far
> > because it unambiguously refers to the exact thing that I want to talk
> > about when discussing the worst case. Other than that, it's a terrible
> > name.
>
> What about "XID allocation overload"? The implication that I'm going
> for here is that the system was misconfigured, or there was otherwise
> some kind of imbalance between XID supply and demand. It also seems to
> convey the true gravity of the situation -- it's *bad*, to be sure,
> but in many environments it's a survivable condition.
>

My concern with the term "overload" is similar to what you expressed below.
It indicates that the situation is due to extra load on the system (or due
to too many XIDs being allocated) and people might assume that the
situation will resolve itself if the load were to be reduced / removed.
However, it's due to that along with some misconfiguration or some other
thing holding back the "removable cutoff".

What do you think about the term "Exhaustion"? Maybe something like "XID
allocation exhaustion" or "Exhaustion of allocatable XIDs"? The term
indicates that we are running out of XIDs to allocate without necessarily
pointing towards a reason.

Regards,
Samay


>
> One possible downside of this name is that it could suggest that all
> that needs to happen is for autovacuum to catch up on vacuuming. In
> reality the user *will* probably have to do more than just wait before
> the system's ability to allocate new XIDs returns, because (in all
> likelihood) autovacuum just won't be able to catch up unless and until
> the user (say) drops a replication slot. Even still, the name seems to
> work; it describes the conceptual model of the system accurately. Even
> before the user drops the replication slot, autovacuum will at least
> *try* to get the system back to being able to allocate new XIDs once
> more.
>
> --
> Peter Geoghegan
>

Re: Overhauling "Routine Vacuuming" docs, particularly its handling of freezing

[samay sharma]

Hi,

On Wed, May 3, 2023 at 2:59 PM Peter Geoghegan  wrote:

> Hi Samay,
>
> On Tue, May 2, 2023 at 11:40 PM samay sharma 
> wrote:
> > Thanks for taking the time to do this. It is indeed difficult work.
>
> Thanks for the review! I think that this is something that would
> definitely benefit from a perspective such as yours.
>

Glad to hear that my feedback was helpful.


>
> > There are things I like about the changes you've proposed and some where
> I feel that the previous section was easier to understand.
>
> That makes sense, and I think that I agree with every point you've
> raised, bar none. I'm pleased to see that you basically agree with the
> high level direction.
>
> I would estimate that the version you looked at (v2) is perhaps 35%
> complete. So some of the individual problems you noticed were a direct
> consequence of the work just not being anywhere near complete. I'll
> try to do a better job of tracking the relative maturity of each
> commit/patch in each commit message, going forward.
>
> Anything that falls under "25.2.1. Recovering Disk Space" is
> particularly undeveloped in v2. The way that I broke that up into a
> bunch of WARNINGs/NOTEs/TIPs was just a short term way of breaking it
> up into pieces, so that the structure was very approximately what I
> wanted. I actually think that the stuff about CLUSTER and VACUUM FULL
> belongs in a completely different chapter. Since it is not "Routine
> Vacuuming" at all.
>
> >> 2. Renamed "Preventing Transaction ID Wraparound Failures" to
> >> "Freezing to manage the transaction ID space". Now we talk about
> >> wraparound as a subtopic of freezing, not vice-versa. (This is a
> >> complete rewrite, as described by later items in this list).
> >
> > +1 on this too. Freezing is a normal part of vacuuming and while the
> aggressive vacuums are different, I think just talking about the worst case
> scenario while referring to it is alarmist.
>
> Strangely enough, Postgres 16 is the first version that instruments
> freezing in its autovacuum log reports. I suspect that some long term
> users will find it quite surprising to see how much (or how little)
> freezing takes place in non-aggressive VACUUMs.
>
> The introduction of page-level freezing will make it easier and more
> natural to tune settings like vacuum_freeze_min_age, with the aim of
> smoothing out the burden of freezing over time (particularly by making
> non-aggressive VACUUMs freeze more). Page-level freezing removes any
> question of not freezing every tuple on a page (barring cases where
> "removable cutoff" is noticeably held back by an old MVCC snapshot).
> This makes it more natural to think of freezing as a process that
> makes it okay to store data in individual physical heap pages, long
> term.
>
> > 1) While I agree that bundling VACUUM and VACUUM FULL is not the right
> way, moving all VACUUM FULL references into tips and warnings also seems
> excessive. I think it's probably best to just have a single paragraph which
> talks about VACUUM FULL as I do think it should be mentioned in the
> reclaiming disk space section.
>
> As I mentioned briefly already, my intention is to move it to another
> chapter entirely. I was thinking of "Chapter 29. Monitoring Disk
> Usage". The "Routine Vacuuming" docs would then link to this sect1 --
> something along the lines of "non-routine commands to reclaim a lot of
> disk space in the event of extreme bloat".
>
> > 2) I felt that the new section, "Freezing to manage the transaction ID
> space" could be made simpler to understand. As an example, I understood
> what the parameters (autovacuum_freeze_max_age, vacuum_freeze_table_age) do
> and how they interact better in the previous version of the docs.
>
> Agreed. I'm going to split it up some more. I think that the current
> "25.2.2.1. VACUUM's Aggressive Strategy" should be split in two, so we
> go from talking about aggressive VACUUMs to Antiwraparound
> autovacuums. Finding the least confusing way of explaining it has been
> a focus of mine in the last few days.
>

To be honest, this was not super simple to understand even in the previous
version. However, as our goal is to simplify this and make it easier to
understand, I'll hold this patch-set to a higher standard :).

I wish there was a simple representation (maybe even a table or something)
which would explain the differences between a VACUUM which is not
aggressive, a VACUUM which ends up being aggressive due to
vacuum_freeze_table_age and an antiwraparound autovacuum.


>
> > 4) I think we should explicitly call out that seeing an anti-wraparound
>

Re: Overhauling "Routine Vacuuming" docs, particularly its handling of freezing

[samay sharma]

Hi,

On Mon, Apr 24, 2023 at 2:58 PM Peter Geoghegan  wrote:

> My work on page-level freezing for PostgreSQL 16 has some remaining
> loose ends to tie up with the documentation. The "Routine Vacuuming"
> section of the docs has no mention of page-level freezing. It also
> doesn't mention the FPI optimization added by commit 1de58df4. This
> isn't a small thing to leave out; I fully expect that the FPI
> optimization will very significantly alter when and how VACUUM
> freezes. The cadence will look quite a lot different.
>
> It seemed almost impossible to fit in discussion of page-level
> freezing to the existing structure. In part this is because the
> existing documentation emphasizes the worst case scenario, rather than
> talking about freezing as a maintenance task that affects physical
> heap pages in roughly the same way as pruning does. There isn't a
> clean separation of things that would allow me to just add a paragraph
> about the FPI thing.
>
> Obviously it's important that the system never enters xidStopLimit
> mode -- not being able to allocate new XIDs is a huge problem. But it
> seems unhelpful to define that as the only goal of freezing, or even
> the main goal. To me this seems similar to defining the goal of
> cleaning up bloat as avoiding completely running out of disk space;
> while it may be "the single most important thing" in some general
> sense, it isn't all that important in most individual cases. There are
> many very bad things that will happen before that extreme worst case
> is hit, which are far more likely to be the real source of pain.
>
> There are also very big structural problems with "Routine Vacuuming",
> that I also propose to do something about. Honestly, it's a huge mess
> at this point. It's nobody's fault in particular; there has been
> accretion after accretion added, over many years. It is time to
> finally bite the bullet and do some serious restructuring. I'm hoping
> that I don't get too much push back on this, because it's already very
> difficult work.
>

Thanks for taking the time to do this. It is indeed difficult work. I'll
give my perspective as someone who has not read the vacuum code but have
learnt most of what I know about autovacuum / vacuuming by reading the
"Routine Vacuuming" page 10s of times.


>
> Attached patch series shows what I consider to be a much better
> overall structure. To make this convenient to take a quick look at, I
> also attach a prebuilt version of routine-vacuuming.html (not the only
> page that I've changed, but the most important set of changes by far).
>
> This initial version is still quite lacking in overall polish, but I
> believe that it gets the general structure right. That's what I'd like
> to get feedback on right now: can I get agreement with me about the
> general nature of the problem? Does this high level direction seem
> like the right one?
>

There are things I like about the changes you've proposed and some where I
feel that the previous section was easier to understand. I'll comment
inline on the summary below and will put in a few points about things I
think can be improved at the end.


>
> The following list is a summary of the major changes that I propose:
>
> 1. Restructures the order of items to match the actual processing
> order within VACUUM (and ANALYZE), rather than jumping from VACUUM to
> ANALYZE and then back to VACUUM.
>
> This flows a lot better, which helps with later items that deal with
> freezing/wraparound.
>

+1


>
> 2. Renamed "Preventing Transaction ID Wraparound Failures" to
> "Freezing to manage the transaction ID space". Now we talk about
> wraparound as a subtopic of freezing, not vice-versa. (This is a
> complete rewrite, as described by later items in this list).
>

+1 on this too. Freezing is a normal part of vacuuming and while the
aggressive vacuums are different, I think just talking about the worst case
scenario while referring to it is alarmist.


>
> 3. All of the stuff about modulo-2^32 arithmetic is moved to the
> storage chapter, where we describe the heap tuple header format.
>
> It seems crazy to me that the second sentence in our discussion of
> wraparound/freezing is still:
>
> "But since transaction IDs have limited size (32 bits) a cluster that
> runs for a long time (more than 4 billion transactions) would suffer
> transaction ID wraparound: the XID counter wraps around to zero, and
> all of a sudden transactions that were in the past appear to be in the
> future"
>
> Here we start the whole discussion of wraparound (a particularly
> delicate topic) by describing how VACUUM used to work 20 years ago,
> before the invention of freezing. That was the last time that a
> PostgreSQL cluster could run for 4 billion XIDs without freezing. The
> invariant is that we activate xidStopLimit mode protections to avoid a
> "distance" between any two unfrozen XIDs that exceeds about 2 billion
> XIDs. So why on earth are we talking about 4 billion XIDs? This is the
> most 

Re: Documentation for building with meson

[samay sharma]

Hi,

On Tue, Apr 11, 2023 at 10:18 AM Andres Freund  wrote:

> Hi,
>
> On 2023-03-28 12:27:26 -0700, samay sharma wrote:
> > Subject: [PATCH v9 1/5] Make minor additions and corrections to meson
> docs
> >
> > This commit makes a few corrections to the meson docs
> > and adds a few instructions and links for better clarity.
> > ---
> >  doc/src/sgml/installation.sgml | 24 +++-
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/doc/src/sgml/installation.sgml
> b/doc/src/sgml/installation.sgml
> > index 70ab5b77d8..e3b9b6c0d0 100644
> > --- a/doc/src/sgml/installation.sgml
> > +++ b/doc/src/sgml/installation.sgml
> > @@ -2057,8 +2057,7 @@ meson setup build -Dssl=openssl
> >  
> >  meson configure -Dcassert=true
> >  
> > -meson configure's commonly used command-line
> options
> > -are explained in .
> > +Commonly used build options for meson configure
> (and meson setup) are explained in  linkend="meson-options"/>.
> > 
> >
> >
> > @@ -2078,6 +2077,13 @@ ninja
> >  processes used with the command line argument -j.
> > 
> >
> > +   
> > +If you want to build the docs, you can type:
> > +
> > +ninja docs
> > +
> > +   
>
> "type" sounds a bit too, IDK, process oriented. "To build the docs, use"?
>

Sure, that works.

>
>
> > Subject: [PATCH v9 2/5] Add data layout options sub-section in
> installation
> >  docs
> >
> > This commit separates out blocksize, segsize and wal_blocksize
> > options into a separate Data layout options sub-section in both
> > the make and meson docs. They were earlier in a miscellaneous
> > section which included several unrelated options. This change
> > also helps reduce the repetition of the warnings that changing
> > these parameters breaks on-disk compatibility.
>
> Makes sense. I'm planning to apply this unless Peter or somebody else has
> further feedback.
>

Cool.

>
>
> > From 11d82aa49efb3d1cbc08f14562a757f115053c8b Mon Sep 17 00:00:00 2001
> > From: Samay Sharma 
> > Date: Mon, 13 Feb 2023 16:23:52 -0800
> > Subject: [PATCH v9 3/5] Remove Anti-Features section from Installation
> from
> >  source docs
> >
> > Currently, several meson setup options are listed in anti-features.
> > However, they are similar to most other options in the postgres
> > features list as they are 'auto' features themselves. Also, other
> > options are likely better suited to the developer options section.
> > This commit, therefore, moves the options listed in the anti-features
> > section into other sections and removes that section.
> >
> > For consistency, this reorganization has been done on the make section
> > of the docs as well.
>
> Makes sense to me. "Anti-Features" is confusing as a name to start with.
>
> Greetings,
>
> Andres Freund
>

Re: Documentation for building with meson

[samay sharma]

Hi,

On Wed, Mar 15, 2023 at 4:28 AM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

>  > [PATCH v8 1/5] Make minor additions and corrections to meson docs
>
> The last hunk revealed that there is some mixing up between meson setup
> and meson configure.  This goes a bit further.  For example, earlier it
> says that to get a list of meson setup options, call meson configure
> --help and look at https://mesonbuild.com/Commands.html#configure, which
> are both wrong.  Also later throughout the text it uses one or the
> other.  I think this has the potential to be very confusing, and we
> should clean this up carefully.
>
> The text about additional meson test options maybe should go into the
> regress.sgml chapter?
>

I tried to make the meson setup and meson configure usage consistent. I've
removed the text for the test options.

>
>
>  > [PATCH v8 2/5] Add data layout options sub-section in installation
>   docs
>
> This makes sense.  Please double check your patch for correct title
> casing, vertical spacing of XML, to keep everything looking consistent.
>

Thanks for noticing. Made it consistent on both sides.

>
> This text isn't yours, but since your patch emphasizes it, I wonder if
> it could use some clarification:
>
> + These options affect how PostgreSQL lays out data on disk.
> + Note that changing these breaks on-disk database compatibility,
> + meaning you cannot use pg_upgrade to upgrade to
> + a build with different values of these options.
>
> This isn't really correct.  What breaking on-disk compatibility means is
> that you can't use a server compiled one way with a data directory
> initialized by binaries compiled another way.  pg_upgrade may well have
> the ability to upgrade between one or the other; that's up to pg_upgrade
> to figure out but not an intrinsic property.  (I wonder why pg_upgrade
> cares about the WAL block size.)
>

 Fixed.

>
>
>  > [PATCH v8 3/5] Remove Anti-Features section from Installation from
>   source docs
>
> Makes sense.  But is "--disable-thread-safety" really a developer
> feature?  I think not.
>
>
Moved to PostgreSQL features. Do you think there's a better place for it?


>
>  > [PATCH v8 4/5] Re-organize Miscellaneous section
>
> This moves the Miscellaneous section after Developer Features.  I think
> Developer Features should be last.
>
> Maybe should remove this section and add the options to the regular
> PostgreSQL Features section.
>

Yes, that makes sense. Made this change.

>
> Also consider the grouping in meson_options.txt, which is slightly
> different yet.


Removed Misc options section from meson_options.txt too.

>
>
>  > [PATCH v8 5/5] Change Short Version for meson installation guide
>
> +# create working directory
> +mkdir postgres
> +cd postgres
> +
> +# fetch source code
> +git clone https://git.postgresql.org/git/postgresql.git src
>
> This comes after the "Getting the Source" section, so at this point they
> already have the source and don't need to do "git clone" etc. again.
>
> +# setup and enter build directory (done only first time)
> +## Unix based platforms
> +meson setup build src --prefix=$PWD/install
> +
> +## Windows
> +meson setup build src --prefix=%cd%/install
>
> Maybe some people work this way, but to me the directory structures you
> create here are completely weird.
>

I'd like to discuss what you think is a good directory structure to work
with. I've mentioned some of the drawbacks I see with the current structure
for the short version. I know this structure can feel different but it
feeling weird is not ideal. Do you have a directory structure in mind which
is different but doesn't feel odd to you?


>
> +# Initialize a new database
> +../install/bin/initdb -D ../data
> +
> +# Start database
> +../install/bin/pg_ctl -D ../data/ -l logfile start
> +
> +# Connect to the database
> +../install/bin/psql -d postgres
>
> The terminology here needs to be tightened up.  You are using "database"
> here to mean three different things.
>

I'll address this together once we are aligned on the overall directory
structure etc.

There are a few reasons why I had done this. Some reasons Andres has
> described in his previous email and I'll add a few specific examples on why
> having the same section for both might not be a good idea.
>
>  * Having readline and zlib as required for building PostgreSQL is now
> wrong because they are not required for meson builds. Also, the name of the
> configs are different for make and meson and the current section only lists
> the make ones.
>  * There are many references to configure in that section which don't
> apply to meson.
>  * Last I checked Flex and Bison were always required to build via meson
> but not for make and the current section doesn't explain those differences.
>
> I spent a good amount of time thinking if we could have a single section,
> clarify these differences to make it correct and not confuse the users. I
> couldn't find a way to do all 

Re: Documentation for building with meson

[samay sharma]

Hi,

On Thu, Dec 1, 2022 at 9:21 AM Andres Freund  wrote:

> Hi,
>
> On 2022-12-01 15:58:39 +0100, Peter Eisentraut wrote:
> > On 23.11.22 22:24, samay sharma wrote:
> > > Thank you. Attaching v7 addressing most of the points below.
> >
> > I have committed this, after some editing and making some structural
> > changes.
>
> Thanks. I was working on that too, but somehow felt a bit stuck...
>
> I'll try if I can adapt my pending changes.
>

I got back to working on the meson docs. I'm attaching a new patch set
proposing some improvements to the current installation docs. I've tried to
make some corrections and improvements suggested in this thread while
trying to maintain consistency across make and meson docs as per Peter's
ask. There are 5 patches in the patch-set:

v8-0001: Makes minor corrections, adds instructions to build docs and adds
a few links to meson docs.
v8-0002, v8-0003 and v8-0004 make changes to restructure the configure
options based on reasoning discussed below. To maintain consistency, I've
made those changes on both the make and meson side.
v8-0005 Reproposes the Short Version I had proposed in v7 as I feel we
should discuss that proposal. I think it improves things in terms of
installation docs. More details below.


>
> > I moved the "Requirements" section back to the top level.  It did
> > not look appealing to have to maintain two copies of this that have
> almost
> > no substantial difference (but for some reason were written with separate
> > structure and wording).
>

There are a few reasons why I had done this. Some reasons Andres has
described in his previous email and I'll add a few specific examples on why
having the same section for both might not be a good idea.

* Having readline and zlib as required for building PostgreSQL is now wrong
because they are not required for meson builds. Also, the name of the
configs are different for make and meson and the current section only lists
the make ones.
* There are many references to configure in that section which don't apply
to meson.
* Last I checked Flex and Bison were always required to build via meson but
not for make and the current section doesn't explain those differences.

I spent a good amount of time thinking if we could have a single section,
clarify these differences to make it correct and not confuse the users. I
couldn't find a way to do all three. Therefore, I think we should move to a
different requirements section for both. I'm happy to re-propose the
previous version which separates them but wanted to see if anybody has
better ideas.


>
> I don't think this is good. The whole "The following software packages are
> required for building PostgreSQL" section is wrong now.  "They are not
> required in the default configuration, but they are needed when certain
> build
> options are enabled, as explained below:" section is misleading as well.
>
> By the time we fix all of those we'll end up with a different section
> again.
>
>
> > Also, I rearranged the Building with Meson section to use the same
> internal
> > structure as the Building with Autoconf and Make section.  This will
> make it
> > easier to maintain going forward.  For example if someone adds a new
> option,
> > it will be easier to find the corresponding places in the lists where to
> add
> > them.


> I don't know. The existing list order makes very little sense to me. The
> E.g. --enable-nls is before the rest in configure, presumably because it
> sorts
> there alphabetically. But that's not the case for meson.
>
> Copying "Anti-Features" as a structure element to the meson docs seems
> bogus
> (also the name is bogus, but that's a pre-existing issue). There's no
> difference in -Dreadline= to the other options meson-options-features list.


> Nor does -Dspinlocks= -Datomics= make sense in the "anti features"
> section. It
> made some sense for autoconf because of the --without- prefix, but that's
> not
> at play in meson.  Their placement in the "Developer Options" made a whole
> lot
> more sense.
>

I agree "Anti-Features" desn't make sense in the meson context. One of my
patches removes that section and moves some options into the "Postgres
Features" section and others into the "Developer Options" section. I've
proposed to make those changes on both sides to make it easier to maintain.


>
> I don't like "Miscellaneous" bit containing minor stuff like krb_srvnam and
> data layout changing options like blocksize,segsize,wal_blocksize. But it
> makes sense to change that for both at the same time.
>

I've proposed a patch to add a new "Data Layout Options" section which
includes: blocksize, segsize and wal_

Re: PGDOCS - Logical replication GUCs - added some xrefs

[samay sharma]

Hi,

On Wed, Dec 7, 2022 at 9:20 PM Peter Smith  wrote:

> On Thu, Dec 8, 2022 at 10:49 AM samay sharma 
> wrote:
> >
> ...
>
> > Thanks for the changes. See a few points of feedback below.
> >
>
> Patch v7 addresses this feedback. PSA.
>
> > > +
> > > + For logical replication,
> publishers
> > > + (servers that do  linkend="sql-createpublication">CREATE
> PUBLICATION)
> > > + replicate data to subscribers
> > > + (servers that do  linkend="sql-createsubscription">CREATE
> SUBSCRIPTION).
> > > + Servers can also be publishers and subscribers at the same time.
> Note,
> > > + the following sections refers to publishers as "senders". The
> parameter
> > > + max_replication_slots has a different meaning
> for the
> > > + publisher and subscriber, but all other parameters are relevant
> only to
> > > + one side of the replication. For more details about logical
> replication
> > > + configuration settings refer to
> > > + .
> > > +
> >
> > The second last line seems a bit odd here. In my last round of feedback,
> I had meant to add the line "The parameter  " onwards to the top of
> logical-replication-config.sgml.
> >
> > What if we made the top of logical-replication-config.sgml like below?
> >
> > Logical replication requires several configuration options to be set.
> Most configuration options are relevant only on one side of the replication
> (i.e. publisher or subscriber). However, max_replication_slots is
> applicable on both sides but has different meanings on each side.
>
>
> OK. Moving this note is not quite following the same pattern as the
> "streaming replication" intro blurb, but anyway it looks fine when
> moved, so I've done as suggested.
>
>
> >> OK. I copied the tablesync note back to config.sgml definition of
> >> 'max_replication_slots' and removed the link as suggested. Frankly, I
> >> also thought it is a bit strange that the max_replication_slots in the
> >> “Sending Servers” section was describing this parameter for
> >> “Subscribers”. OTOH, I did not want to split the definition in half so
> >> instead, I’ve added another Subscriber  that just refers
> >> back to this place. It looks like an improvement to me.
> >
> >
> > Hmm, I agree this is a tricky scenario. However, to me, it seems odd to
> mention the parameter twice as this chapter of the docs just lists each
> parameter and describes them. So, I'd probably remove the reference to it
> in the subscriber section. We should describe it's usage in different
> places in the logical replication part of the docs (as we do).
>
> The 'max_replication_slots' is problematic because it is almost like
> having 2 different GUCs that happen to have the same name. So I
> preferred it also gets a mention in the “Subscriber” section to make
> it obvious that it wears 2 hats, but IIUC you prefer that 2nd mention
> is not present because typically each GUC should appear once only in
> this chapter. TBH, I think both ways could be successfully argued for
> or against -- so I’m just going to leave this as-is for now and let
> the committer decide.
>

Sounds fair.

I don't have any other feedback. This looks good to me.

Also, I don't see this patch in the 2023/01 commitfest. Might be worth
moving to that one.

Regards,
Samay
Microsoft


>
> > > +   
> > > + linkend="guc-max-logical-replication-workers">max_logical_replication_workers
> > > +must be set to at least the number of subscriptions (for apply
> workers), plus
> > > +some reserve for the table synchronization workers. Configuration
> parameter
> > > + linkend="guc-max-worker-processes">max_worker_processes
> > > +may need to be adjusted to accommodate for replication workers,
> at least (
> > > + linkend="guc-max-logical-replication-workers">max_logical_replication_workers
> > > ++ 1). Note, some extensions and parallel
> queries also
> > > +take worker slots from max_worker_processes.
> > > +   
> >
> > Maybe do max_worker_processes in a new line like the rest.
>
> OK. Done as suggested.
>
> --
> Kind Regards,
> Peter Smith.
> Fujitsu Australia
>

Re: PGDOCS - Logical replication GUCs - added some xrefs

[samay sharma]

Hi,

On Tue, Dec 6, 2022 at 11:12 PM Peter Smith  wrote:

> On Tue, Dec 6, 2022 at 5:57 AM samay sharma 
> wrote:
> >
> > Hi,
> >
> > On Mon, Oct 24, 2022 at 12:45 AM Peter Smith 
> wrote:
> >>
> >> Hi hackers.
> >>
> >> There is a docs Logical Replication section "31.10 Configuration
> >> Settings" [1] which describes some logical replication GUCs, and
> >> details on how they interact with each other and how to take that into
> >> account when setting their values.
> >>
> >> There is another docs Server Configuration section "20.6 Replication"
> >> [2] which lists the replication-related GUC parameters, and what they
> >> are for.
> >>
> >> Currently AFAIK those two pages are unconnected, but I felt it might
> >> be helpful if some of the parameters in the list [2] had xref links to
> >> the additional logical replication configuration information [1]. PSA
> >> a patch to do that.
> >
> >
> > +1 on the patch. Some feedback on v5 below.
> >
>
> Thanks for your detailed review comments!
>
> I have changed most things according to your suggestions. Please check
> patch v6.
>

Thanks for the changes. See a few points of feedback below.

> +
> + For logical replication,
publishers
> + (servers that do CREATE
PUBLICATION)
> + replicate data to subscribers
> + (servers that do CREATE
SUBSCRIPTION).
> + Servers can also be publishers and subscribers at the same time.
Note,
> + the following sections refers to publishers as "senders". The
parameter
> + max_replication_slots has a different meaning
for the
> + publisher and subscriber, but all other parameters are relevant
only to
> + one side of the replication. For more details about logical
replication
> + configuration settings refer to
> + .
> +

The second last line seems a bit odd here. In my last round of feedback, I
had meant to add the line "The parameter  " onwards to the top of
logical-replication-config.sgml.

What if we made the top of logical-replication-config.sgml like below?

Logical replication requires several configuration options to be set. Most
configuration options are relevant only on one side of the replication
(i.e. publisher or subscriber). However, max_replication_slots is
applicable on both sides but has different meanings on each side.

>
> > > +
> > > + For logical replication configuration
> settings refer
> > > + also to .
> > > +
> > > +
> >
> > I feel the top paragraph needs to explain terminology for logical
> replication like it does for physical replication in addition to linking to
> the logical replication config page. I'm recommending this as we use terms
> like subscriber etc. in description of parameters without introducing them
> first.
> >
> > As an example, something like below might work.
> >
> > These settings control the behavior of the built-in streaming
> replication feature (see Section 27.2.5) and logical replication (link).
> >
> > For physical replication, servers will be either a primary or a standby
> server. Primaries can send data, while standbys are always receivers of
> replicated data. When cascading replication (see Section 27.2.7) is used,
> standby servers can also be senders, as well as receivers. Parameters are
> mainly for sending and standby servers, though some parameters have meaning
> only on the primary server. Settings may vary across the cluster without
> problems if that is required.
> >
> > For logical replication, servers will either be publishers (also called
> senders in the sections below) or subscribers. Publishers are ,
> Subscribers are...
> >
>
> OK. I split this blurb into 2 parts – streaming and logical
> replication. The streaming replication part is the same as before. The
> logical replication part is new.
>
> > > +   
> > > + See  for more
> details
> > > + about setting max_replication_slots for
> logical
> > > + replication.
> > > +
> >
> >
> > The link doesn't add any new information regarding max_replication_slots
> other than "to reserve some for table sync" and has a good amount of
> unrelated info. I think it might be useful to just put a line here asking
> to reserve some for table sync instead of linking to the entire logical
> replication config section.
> >
>
> OK. I copied the tablesync note back to config.sgml definition of
> 'max_replication_slots' and removed the link as suggested.

Re: PGDOCS - Logical replication GUCs - added some xrefs

[samay sharma]

Hi,

On Mon, Oct 24, 2022 at 12:45 AM Peter Smith  wrote:

> Hi hackers.
>
> There is a docs Logical Replication section "31.10 Configuration
> Settings" [1] which describes some logical replication GUCs, and
> details on how they interact with each other and how to take that into
> account when setting their values.
>
> There is another docs Server Configuration section "20.6 Replication"
> [2] which lists the replication-related GUC parameters, and what they
> are for.
>
> Currently AFAIK those two pages are unconnected, but I felt it might
> be helpful if some of the parameters in the list [2] had xref links to
> the additional logical replication configuration information [1]. PSA
> a patch to do that.
>

+1 on the patch. Some feedback on v5 below.

> +
> + For logical replication configuration
settings refer
> + also to .
> +
> +

I feel the top paragraph needs to explain terminology for logical
replication like it does for physical replication in addition to linking to
the logical replication config page. I'm recommending this as we use terms
like subscriber etc. in description of parameters without introducing them
first.

As an example, something like below might work.

These settings control the behavior of the built-in streaming replication
feature (see Section 27.2.5) and logical replication (link).

For physical replication, servers will be either a primary or a standby
server. Primaries can send data, while standbys are always receivers of
replicated data. When cascading replication (see Section 27.2.7) is used,
standby servers can also be senders, as well as receivers. Parameters are
mainly for sending and standby servers, though some parameters have meaning
only on the primary server. Settings may vary across the cluster without
problems if that is required.

For logical replication, servers will either be publishers (also called
senders in the sections below) or subscribers. Publishers are ,
Subscribers are...

> +   
> + See  for more
details
> + about setting max_replication_slots for
logical
> + replication.
> +


The link doesn't add any new information regarding max_replication_slots
other than "to reserve some for table sync" and has a good amount of
unrelated info. I think it might be useful to just put a line here asking
to reserve some for table sync instead of linking to the entire logical
replication config section.

> -   Logical replication requires several configuration options to be set.
> +   Logical replication requires several configuration parameters to be
set.

May not be needed? The docs have references to both options and parameters
but I don't feel strongly about it. Feel free to use what you prefer.

I think we should add an additional line to the intro here saying that
parameters are mostly relevant only one of the subscriber or publisher.
Maybe a better written version of "While max_replication_slots means
different things on the publisher and subscriber, all other parameters are
relevant only on either the publisher or the subscriber."

> +  
> +   Notes

I don't think we need this sub-section. If I understand correctly, these
parameters are effective only on the subscriber side. So, any reason to not
include them in that section?

> +
> +   
> +Logical replication workers are also affected by
> +wal_receiver_timeout,
> +wal_receiver_status_interval
and
> +wal_receiver_retry_interval.
> +   
> +

I like moving this; it makes more sense here. Should we remove it from
config.sgml? It seems a bit out of place there as we generally talk only
about individual parameters there and this line is general logical
replication subscriber advise which is more suited to
logical-replication.sgml

> +   
> +Configuration parameter
> +max_worker_processes
> +may need to be adjusted to accommodate for replication workers, at
least (
> +max_logical_replication_workers
> ++ 1). Some extensions and parallel queries also
take
> +worker slots from max_worker_processes.
> +   
> +
> +  

I think we should move this to the subscriber section as said above. It's
useful to know this and people might skip over the notes.


> ~~
>
> Meanwhile, I also suspect that the main blurb top of [1] is not
> entirely correct... it says "These settings control the behaviour of
> the built-in streaming replication feature", although some of the GUCs
> mentioned later in this section are clearly for "logical replication".
>

> Thoughts?
>

I shared an idea above.

Regards,
Samay


>
> --
> [1] 31.10 Configuration Settings -
> https://www.postgresql.org/docs/current/logical-replication-config.html
> [2] 20.6 Replication -
> https://www.postgresql.org/docs/current/runtime-config-replication.html
>
> Kind Regards,
> Peter Smith.
> Fujitsu Australia
>

Re: Documentation for building with meson

[samay sharma]

Hi,

On Wed, Nov 23, 2022 at 12:16 PM Justin Pryzby  wrote:

> On Wed, Nov 23, 2022 at 11:30:54AM -0800, samay sharma wrote:
> > Thanks for the feedback. Addressed all and added markup at a few more
> > places in v6 (attached).
>
> Thanks.  It looks good to me.  A couple thoughts, maybe they're not
> important.
>

Thank you. Attaching v7 addressing most of the points below.


>
>  - LZ4 and Zstd refer to wal_compression and default_toast_compression,
>but not to any corresponding option to basebackup.
>
>  - There's no space after the hash mark here; but above, there was:
>#Setup build directory with a different installation prefix
>

Added a space as that looks better.

>
>  - You use slash to show enumerated options, but it's more typical to
>use braces: {a | b | c}:
>-Dnls=auto/enabled/disabled
>

Changed.

>
>  - There's no earlier description/definition of an "auto" feature, but
>still says this:
>"Setting this option allows you to override value of all 'auto'
> features"
>

Described what an "auto" feature is in ().

>
>  - Currently the documentation always refers to "PostgreSQL", but you
>added two references to "Postgres":
>+ If a program required to build Postgres...
>+ Once Postgres is built...
>

Good catch. Changed to PostgreSQL.

Regards,
Samay

>
> --
> Justin
>


v7-0001-Add-docs-for-building-with-meson.patch
Description: Binary data

Re: Documentation for building with meson

[samay sharma]

Hi,

On Tue, Nov 22, 2022 at 10:36 PM Justin Pryzby  wrote:

> On Mon, Nov 14, 2022 at 10:41:21AM -0800, samay sharma wrote:
>
> > You need LZ4, if you want to support compression of data with that
> > method; see default_toast_compression and wal_compression.
>
> => The first comma is odd.  Maybe it should say "LZ4 is needed to
> support .."
>
> > You need Zstandard, if you want to support compression of data or
> > backups with that method; see wal_compression. The minimum required
> > version is 1.4.0.
>
> Same.
>
> Also, since v15, LZ4 and zstd can both be used by basebackup.
>
> >Some commonly used ones are mentioned in the subsequent sections
>
> => Some commonly used options ...
>
> >  Most of these require additional software, as described in Section
> >  17.3.2, and are set to be auto features.
>
> => "Are set to be auto features" sounds odd.  I think it should say
> something like " .. and are automatically enabled if the required
> software is detected.".
>
> > You can change this behavior by manually setting the auto features to
> > enabled to require them or disabled to not build with them.
>
> remove "auto".  Maybe "enabled" and "disabled" need markup.
>
> > On Windows, the default WinLDAP library is used. It defults to auto
>
> typo: defults
>
> > It defaults to auto and libsystemd and the associated header files need
> > to be installed to use this option.
>
> => write this as two separate sentences.  Same for libxml.
>
> > bsd to use the UUID functions found in FreeBSD, NetBSD, and some other
> > BSD-derived systems
>
> => should remove mention of netbsd, like c4b6d218e
>
> > Enables use of the Zlib library. It defaults to auto and enables
> > support for compressed archives in pg_dump ,pg_restore and
> > pg_basebackup and is recommended.
>
> => The comma is mis-placed.
>
> > The default backend meson uses is ninja and that should suffice for
> > most use cases. However, if you'd like to fully integrate with
> > visual studio, you can set the BACKEND to vs.
>
> => BACKEND is missing markup.
>
> > This option can be used to specify the buildtype to use; defaults to
> > release
>
> => release is missing markup
>
> >  Specify the optimization level. LEVEL can be set to any of
> >  {0,g,1,2,3,s}.
>
> => LEVEL is missing markup
>

Thanks for the feedback. Addressed all and added markup at a few more
places in v6 (attached).

Regards,
Samay

>
> Thanks,
> --
> Justin
>


v6-0001-Add-docs-for-building-with-meson.patch
Description: Binary data

Re: Documentation for building with meson

[samay sharma]

Hi,

On Thu, Nov 10, 2022 at 4:46 AM Nazir Bilal Yavuz 
wrote:

> Hi,
>
> I did some tests on windows. I used 'ninja' as a backend.
> On 11/8/2022 9:23 PM, samay sharma wrote:
>
> Hi,
>
> On Sat, Nov 5, 2022 at 2:39 PM Andres Freund  wrote:
>
>> Hi,
>>
>> On 2022-10-30 20:51:59 -0700, samay sharma wrote:
>> > +# setup and enter build directory (done only first time)
>> > +meson setup build src --prefix=$PWD/install
>>
>> This command won't work on windows, I think.
>>
>
> Yes, $PWD isn't recognized on windows, %CD% could be alternative.
>
Added.

>
> > +  
>> > +
>>  
>> --sysconfdir=DIRECTORY
>> > +   
>> > +
>> > + Sets the directory for various configuration files,
>> > + PREFIX/etc by
>> default.
>> > +
>> > +   
>> > +  
>>
>> Need to check what windows does here.
>>
>
> It is same on windows: 'PREFIX/etc'.
>
> I also checked other dirs(bindir, sysconfdir, libdir, includedir, datadir,
> localedir, mandir), default path is correct for all of them.
>

Thanks. I've made a few windows specific fixes in the latest version.
Attached v5.

Regards,
Samay


>
> Regards,
> Nazir Bilal Yavuz
>
>
>
>


v5-0001-Add-docs-for-building-with-meson.patch
Description: Binary data

Re: Documentation for building with meson

[samay sharma]

Hi,

On Thu, Oct 27, 2022 at 1:04 AM John Naylor 
wrote:

> +# Run the main pg_regress and isolation tests
> +meson test --suite main
>
> This does not work for me in a fresh install until running
>
> meson test --suite setup
>
> In fact, we see in
>
> https://wiki.postgresql.org/wiki/Meson
>
> meson test --suite setup --suite main
>

You are right that this will be needed for a new install. I've added
--suite setup in the testing section in the v3 of the patch (attached).


> That was just an eyeball check from a naive user -- it would be good to
> try running everything documented here.
>

I retried all the instructions as suggested and they work for me.

Regards,
Samay


>
> --
> John Naylor
> EDB: http://www.enterprisedb.com
>


v3-0001-Add-docs-for-building-with-meson.patch
Description: Binary data

Re: Documentation for building with meson

[samay sharma]

Hi,

On Wed, Oct 19, 2022 at 7:43 PM Justin Pryzby  wrote:

> On Wed, Oct 19, 2022 at 11:35:10AM -0700, samay sharma wrote:
> > Creating a new thread focussed on adding docs for building Postgres with
> > meson. This is a spinoff from the original thread [1] and I've attempted
> to
> > address all the feedback provided there in the attached patch.
> >
> > Please let me know your thoughts.
>
> It's easier to review rendered documentation.
> I made a rendered copy available here:
>
> https://api.cirrus-ci.com/v1/artifact/task/6084297781149696/html_docs/html_docs/install-meson.html


Thanks for your for review. Attached v2 of the patch here.


>
>
> +  Flex and Bison
> +  are needed to build PostgreSQL using
> +  meson. Be sure to get
> +  Flex 2.5.31 or later and
> +  Bison 1.875 or later from your package
> manager.
> +  Other lex and
> yacc
> +  programs cannot be used.
>
> These versions need to be updated, see also: 57bab3330:
>   - b086a47a270 "Bump minimum version of Bison to 2.3"
>   - 8b878bffa8d "Bump minimum version of Flex to 2.5.35"
>

Changed


>
> + will be enabled automatically if the required packages are found.
>
> should refer to files/libraries/headers/dependencies rather than
> "packages" ?
>

Changed to dependencies


>
> + default is false that is to use
> Readline.
>
> "that is to use" should be parenthesized or separate with commas, like
> | default is false, that is to use Readline.
>
> zlib is mentioned twice, the first being "strongly recommended".
> Is that intended?  Also, basebackup can use zlib, too.
>

Yes, the first is in the requirements section where we just list packages
required / recommended. The other mention is in the list of configure
options. This is similar to how the documentation looks today for make /
autoconf. Added pg_basebackup as a use case too.


>
> + If you have the binaries for certain by programs required to
> build
>
> remove "by" ?
>

Done


>
> + Postgres (with or without optional flags) stored at non-standard
> + paths, you could specify them manually to meson configure. The
> complete
> + list of programs for whom this is supported can be found by
> running
>
> for *which
>
> Actually, I suggest to say:
> |If a program required to build Postgres (with or without optional flags)
> |is stored in a non-standard path, ...
>

Liked this framing better. Changed.

>
> + a build with a different value of these options.
>
> .. with different values ..
>

Done

>
> + the server, it is recommended to use atleast the
> --buildtype=debug
>
> at least
>
Done

>
> +and it's options in the meson documentation.
>
> its
>
Done

>
> Maybe other things should have  ?
>
>  Git
>  Libxml2
>  libxslt
>  visual studio
>  DTrace
>  ninja
>
> +  Flex and Bison
>
> Maybe these should use  ?
>

I'm unsure of the right protocol for this. I tried to follow the precedent
set in the make / autoconf part of the documentation, which uses
 at certain places and  at others. Is there a
reference or guidance on which to use where or is it mostly a case by case
decision?


> +  be installed with pip.
>
> Should be  ?
>

Changed.

>
> This part is redundant with prior text:
> " To use this option, you will need an implementation of the Gettext API. "
>

Modified.

>
> + Enabls use of the Zlib library
>
> typo: Enabls
>

Fixed.

>
> + This option is set to true by default and setting it to false will
>
> change "and" to ";" for spinlocks and atomics?
>

Done

>
> + Debug info is generated but the result is not optimized.
>
> Maybe say the "code" is not optimized ?
>

Changed

>
> + the tests can slow down the server significantly
>
> remove "can"
>

Done.


>
> + You can override the amount of parallel processes used with
>
> s/amount/number/
>

Done

>
> + If you'd like to build with a backend other that ninja
>
> other *than
>

Fixed.

>
> +  the GNU C library then you will additionally
>
> library comma
>

Added

>
> +argument. If no srcdir is given Meson will deduce
> the
>
> given comma
>

Added

>
> + It should be noted that after the initial configure step
>
> step comma
>

Added

>
> +After the installation you can free disk space by removing the built
>
> installation comma
>

Added

>
> +To learn more about running the tests and how to interpret the results
> +you can refer to t

Documentation for building with meson

[samay sharma]

Hi,

Creating a new thread focussed on adding docs for building Postgres with
meson. This is a spinoff from the original thread [1] and I've attempted to
address all the feedback provided there in the attached patch.

Please let me know your thoughts.

[1]
https://www.postgresql.org/message-id/28de92b5-a514-fe1b-1637-ba228aa2cccf%40enterprisedb.com

Regards,
Samay


v1-0001-Add-docs-for-building-with-meson.patch
Description: Binary data

Re: [RFC] building postgres with meson - v13

[samay sharma]

Hi,

On Mon, Sep 26, 2022 at 6:02 AM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 24.09.22 20:09, Andres Freund wrote:
> > On 2022-09-24 13:52:29 -0400, Tom Lane wrote:
> >> ... btw, shouldn't the CF entry [1] get closed now?
> >
> > Unfortunately not - there's quite a few followup patches that haven't
> been
> > [fully] reviewed and thus not applied yet.
>
> Here is some review of the remaining ones (might not match exactly what
> you attached, I was working off your branch):
>
>
> 9f789350a7a7 meson: ci: wip: move compilerwarnings task to meson
>
> This sounds reasonable to me in principle, but I haven't followed the
> cirrus stuff too closely, and it doesn't say why it's "wip".  Perhaps
> others could take a closer look.
>
>
> ccf20a68f874 meson: ci: Add additional CI coverage
>
> IIUC, this is just for testing your branch, not meant for master?
>
>
> 02d84c21b227 meson: prereq: win: remove date from version number in
> win32ver.rc
>
> do it
>
>
> 5c42b3e7812e meson: WIP: Add some of the windows resource files
>
> What is the thinking on this now?  What does this change over the
> current state?
>
>
> 9bc60bccfd10 meson: Add support for relative rpaths, fixing tests on
> MacOS w/ SIP
>
> I suggest a separate thread and/or CF entry for this.  There have been
> various attempts to deal with SIP before, with varying results.  This
> is not part of the meson transition as such.
>
>
> 9f5be26c1215 meson: Add docs for building with meson
>
> I do like the overall layout of this.
>
> The "Supported Platforms" section should be moved back to near the end
> of the chapter.  I don't see a reason to move it forward, at least
> none that is related to the meson issue.
>

Agreed that it's unrelated to meson. However, I think it's better to move
it in the front as it's generally useful to know if your platform is
supported before you start performing the installation steps and get stuck
somewhere.

Do you think I should submit that as a separate commit in the same
patch-set or just move it out to a completely different patch submission?


>
> The changes to the "Getting the Source" section are also not
> appropriate for this patch.
>
>
Given that many developers are now using Git for downloading the source
code, I think it makes sense to be in the Getting the source section. Also,
meson today doesn't cleanly build via the tarballs. Hence, I added it to
the section (and patchset).

Do you think I should move this to a different patch?


> In the section "Building and Installation with meson":
>
> - Remove the "git clone" stuff.


> - The "Running tests" section should be moved to Chapter 33. Regression
> Tests.
>

The autoconf / make section also has a small section on how to run the
regression tests. The "Running tests" section is meant to the be equivalent
of that for meson (i.e. brief overview). I do intend to add a detailed
section to Chapter 33 with more info on how to interpret test results etc.
Do you think the current section is too verbose for where it is?


> Some copy-editing will probably be suitable, but I haven't focused on
> that yet.
>
>
> 9c00d355d0e9 meson: Add PGXS compatibility
>
> This looks like a reasonable direction to me.  How complete is it?  It
> says it works for some extensions but not others.  How do we define
> the target line here?
>
>
> 3fd5e13dcad3 meson: Add postgresql-extension.pc for building extension
> libraries
>
> Separate thread for this as well.  This is good and important, but we
> must also add it to the make build.
>
>
> 4b5bfa1c19aa meson: Add LLVM bitcode emission
>
> still in progress
>
>
> eb40f6e53104 meson: Add support for building with precompiled headers
>
> Any reason not to enable this by default?  The benefits on non-Windows
> appear to be less dramatic, but they are not zero.  Might be better to
> enable it consistently so that for example any breakage is easier
> caught.
>
>
> 377bfdea6042 meson: Add xmllint/xsltproc wrapper script to handle
> dependencies automatically
>
> Is this part of the initial transition, required for correctness, or
> is it an optional optimization?  Could use more explanation.  Maybe
> move to separate thread also?
>
>

Re: [RFC] building postgres with meson - v11

[samay sharma]

On Tue, Sep 6, 2022 at 9:46 PM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 02.09.22 01:12, samay sharma wrote:
> > So, I was thinking of the following structure:
> > - Supported Platforms
> > - Getting the Source
> > - Building with make and autoconf
> >-- Short version
> >-- Requirements
> >-- Installation Procedure and it's subsections
> > - Building with Meson
> >-- Short version
> >-- Requirements
> >-- Installation Procedure and it's subsections
> > - Post-installation Setup
> > - Platform specific notes
>
> I like that.
>

Attached is a docs-only patch with that structure. We need to update the
platform specific notes section to add meson specific nuances. Also, in
terms of supported platforms, if there are platforms which work with make
but not with meson, we have to add that too.

Regards,
Samay

>
> > As a follow up patch, we could also try to fit the Windows part into
> > this model. We could add a Building with visual C++ or Microsoft windows
> > SDK section. It doesn't have a short version but follows the remaining
> > template of requirements and installation procedure subsections
> > (Building, Cleaning and Installing and Running Regression tests) well.
>
> We were thinking about removing the old Windows build system for PG 16.
> Let's see how that goes.  Otherwise, yes, that would be good as well.
>


v1-0001-Add-meson-docs-to-existing-build-from-source.patch
Description: Binary data

Re: [RFC] building postgres with meson - v11

[samay sharma]

Hi,

On Tue, Sep 6, 2022 at 9:48 PM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 02.09.22 19:16, samay sharma wrote:
> > Another thing I'd like input on. A common question I've heard from
> > people who've tried out the docs is How do we do the equivalent of X in
> > make with meson. As meson will be new for a bunch of people who are
> > fluent with make, I won't be surprised if this is a common ask. To
> > address that, I was planning to add a page to specify the key things one
> > needs to keep in mind while "migrating" from make to meson and having a
> > translation table of commonly used commands.
> >
> > I was planning to add it in the meson section, but if we go ahead with
> > the structure proposed above, it doesn't fit it into one as cleanly.
> > Maybe, it still goes in the meson section? Thoughts?
>
> This could go into the wiki.
>
> For example, we have
> <https://wiki.postgresql.org/wiki/Working_with_Git>, which was added
> during the CVS->Git transition.
>

That's a good idea. I'll add a page to the wiki about this topic and share
it on the list for review.


>
> This avoids that we make the PostgreSQL documentation a substitute
> manual for a third-party product.
>

Regards,
Samay

Re: [RFC] building postgres with meson - v11

[samay sharma]

On Thu, Sep 1, 2022 at 4:12 PM samay sharma  wrote:

> Hi,
>
> On Wed, Aug 31, 2022 at 1:42 AM Peter Eisentraut <
> peter.eisentr...@enterprisedb.com> wrote:
>
>> On 24.08.22 17:30, Andres Freund wrote:
>> >> 0545eec895 meson: Add docs
>> >>
>> >> We should think more about how to arrange the documentation.  We
>> >> probably don't want to copy-and-paste all the introductory and
>> >> requirements information.  I think we can make this initially much
>> >> briefer, like the Windows installation chapter.  For example, instead
>> >> of documenting each setup option again, just mention which ones exist
>> >> and then point (link) to the configure chapter for details.
>> > The current docs, including the windows ones, are already hard to
>> follow. I
>> > think we should take some care to not make the meson bits even more
>> > confusing. Cross referencing left and right seems problematic from that
>> angle.
>>
>> If you look at the current structure of the installation chapter
>>
>> 17.1. Short Version
>> 17.2. Requirements
>> 17.3. Getting the Source
>> 17.4. Installation Procedure
>> 17.5. Post-Installation Setup
>> 17.6. Supported Platforms
>> 17.7. Platform-Specific Notes
>>
>> only 17.1, small parts of 12.2, and 17.4 should differ between make and
>> meson.  There is no conceivable reason why the meson installation
>> chapter should have a different "Getting the Source" section.  And some
>> of the post-installation and platform-specific information doesn't
>> appear at all on the meson chapter.
>>
>> I think we can try to be a bit more ingenious in how we weave this
>> together in the best way.  What I really wouldn't want is two separate
>> chapters that duplicate the entire process.  I think we could do one
>> chapter, like
>>
>> - Short Version
>> - Requirements
>> - Getting the Source
>> - Installation Procedure
>> - Installation Procedure using Meson
>> - Post-Installation Setup
>> - Supported Platforms
>> - Platform-Specific Notes
>>
>
> I spent some more time thinking about the structure of the docs. The
> getting the source, supported platforms, post installation setup and
> platform specific notes sections are going to be mostly common. We do
> expect some differences in supported platforms and platform specific notes
> but I think they should be manageable without confusing readers.
>
> The others; short version, requirements, and installation procedure are
> pretty different and I feel combining them will end up confusing readers or
> require creating autoconf / make and meson versions of many things at many
> different places. Also, if we keep it separate, it'll be easier to remove
> make / autoconf specific sections if (when?) we want to do that.
>
> So, I was thinking of the following structure:
> - Supported Platforms
> - Getting the Source
> - Building with make and autoconf
>   -- Short version
>   -- Requirements
>   -- Installation Procedure and it's subsections
> - Building with Meson
>   -- Short version
>   -- Requirements
>   -- Installation Procedure and it's subsections
> - Post-installation Setup
> - Platform specific notes
>
> It has the disadvantage of short version moving to a bit later in the
> chapter but I think it's a good structure to reduce duplication and also
> keep sections which are different separate. Thoughts on this approach? If
> this looks good, I can submit a patch rearranging things this way.
>

Another thing I'd like input on. A common question I've heard from people
who've tried out the docs is How do we do the equivalent of X in make with
meson. As meson will be new for a bunch of people who are fluent with make,
I won't be surprised if this is a common ask. To address that, I was
planning to add a page to specify the key things one needs to keep in mind
while "migrating" from make to meson and having a translation table of
commonly used commands.

I was planning to add it in the meson section, but if we go ahead with the
structure proposed above, it doesn't fit it into one as cleanly. Maybe, it
still goes in the meson section? Thoughts?

Regards,
Samay


>
> As a follow up patch, we could also try to fit the Windows part into this
> model. We could add a Building with visual C++ or Microsoft windows SDK
> section. It doesn't have a short version but follows the remaining template
> of requirements and installation procedure subsections (Building, Cleaning
> and Installing and Running Regression tests) well.
>
> Regards,
> Samay
>
>>
>> Alternatively, if people prefer two separate chapters, let's think about
>> some source-code level techniques to share the common contents.
>>
>

Re: [RFC] building postgres with meson - v11

[samay sharma]

Hi,

On Wed, Aug 31, 2022 at 1:42 AM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 24.08.22 17:30, Andres Freund wrote:
> >> 0545eec895 meson: Add docs
> >>
> >> We should think more about how to arrange the documentation.  We
> >> probably don't want to copy-and-paste all the introductory and
> >> requirements information.  I think we can make this initially much
> >> briefer, like the Windows installation chapter.  For example, instead
> >> of documenting each setup option again, just mention which ones exist
> >> and then point (link) to the configure chapter for details.
> > The current docs, including the windows ones, are already hard to
> follow. I
> > think we should take some care to not make the meson bits even more
> > confusing. Cross referencing left and right seems problematic from that
> angle.
>
> If you look at the current structure of the installation chapter
>
> 17.1. Short Version
> 17.2. Requirements
> 17.3. Getting the Source
> 17.4. Installation Procedure
> 17.5. Post-Installation Setup
> 17.6. Supported Platforms
> 17.7. Platform-Specific Notes
>
> only 17.1, small parts of 12.2, and 17.4 should differ between make and
> meson.  There is no conceivable reason why the meson installation
> chapter should have a different "Getting the Source" section.  And some
> of the post-installation and platform-specific information doesn't
> appear at all on the meson chapter.
>
> I think we can try to be a bit more ingenious in how we weave this
> together in the best way.  What I really wouldn't want is two separate
> chapters that duplicate the entire process.  I think we could do one
> chapter, like
>
> - Short Version
> - Requirements
> - Getting the Source
> - Installation Procedure
> - Installation Procedure using Meson
> - Post-Installation Setup
> - Supported Platforms
> - Platform-Specific Notes
>

I spent some more time thinking about the structure of the docs. The
getting the source, supported platforms, post installation setup and
platform specific notes sections are going to be mostly common. We do
expect some differences in supported platforms and platform specific notes
but I think they should be manageable without confusing readers.

The others; short version, requirements, and installation procedure are
pretty different and I feel combining them will end up confusing readers or
require creating autoconf / make and meson versions of many things at many
different places. Also, if we keep it separate, it'll be easier to remove
make / autoconf specific sections if (when?) we want to do that.

So, I was thinking of the following structure:
- Supported Platforms
- Getting the Source
- Building with make and autoconf
  -- Short version
  -- Requirements
  -- Installation Procedure and it's subsections
- Building with Meson
  -- Short version
  -- Requirements
  -- Installation Procedure and it's subsections
- Post-installation Setup
- Platform specific notes

It has the disadvantage of short version moving to a bit later in the
chapter but I think it's a good structure to reduce duplication and also
keep sections which are different separate. Thoughts on this approach? If
this looks good, I can submit a patch rearranging things this way.

As a follow up patch, we could also try to fit the Windows part into this
model. We could add a Building with visual C++ or Microsoft windows SDK
section. It doesn't have a short version but follows the remaining template
of requirements and installation procedure subsections (Building, Cleaning
and Installing and Running Regression tests) well.

Regards,
Samay

>
> Alternatively, if people prefer two separate chapters, let's think about
> some source-code level techniques to share the common contents.
>

Re: [RFC] building postgres with meson - v11

[samay sharma]

Hi,

On Wed, Aug 24, 2022 at 8:30 AM Andres Freund  wrote:

> Hi,
>
> On 2022-08-24 11:39:06 +0200, Peter Eisentraut wrote:
> > I have looked at your branch at 0545eec895:
> >
> > 258f6dc0a7 Don't hardcode tmp_check/ as test directory for tap tests
> > 8ecc33cf04 Split TESTDIR into TESTLOGDIR and TESTDATADIR
> >
> > I think these patches are split up a bit incorrectly.  If you apply
> > the first patch by itself, then the output appears in tab_comp_dir/
> > directly under the source directory.  And then the second patch moves
> > it to tmp_check/tap_comp_dir/.  If there is an intent to apply these
> > patches separately somehow, this should be cleaned up.
>
> How is that happening with that version of the patch? The test puts
> tap_comp_dir under TESTDIR, and TESTDIR is $(CURDIR)/tmp_check. There was
> an
> earlier version of the patch that was split one more time that did have
> that
> problem, but I don't quite see how that version has it?
>
>
> > I haven't checked the second patch in detail yet, but it looks like
> > the thought was that the first patch is about ready to go.
> >
> > 834a40e609 meson: prereq: Extend gendef.pl in preparation for meson
> >
> > I'm not qualified to check that in detail, but it looks reasonable
> > enough to me.
> >
> > See attached patch (0001) for a perlcritic fix.
>
> Thanks.
>
>
> > 97a0b096e8 meson: prereq: Add src/tools/gen_export.pl
> >
> > This produces leading whitespace in the output files that at least on
> > darwin wasn't there before.  See attached patch (0002).  This should
> > be checked again on other platforms as well.
>
> Hm, to me the indentation as is makes more sense, but ...
>
> > Other than that this looks good.  Attached is a small cosmetic patch
> (0003).
>
> I wonder if we should rewrite this in python - I chose perl because I
> thought
> we could share it, but as you pointed out, that's not possible, because we
> don't want to depend on perl during the autoconf build from a tarball.
>
>
> > e0a8387660 solaris: Use versioning scripts instead of -Bsymbolic
> >
> > This looks like a good idea.  The documentation clearly states that
> > -Bsymbolic shouldn't be used, at least not in the way we have been
> > doing.  Might as well go ahead with this and give it a whirl on the
> > build farm.
>
> Cool. I looked at this because I was confused about getting warnings with
> autoconf that I wasn't getting with meson.
>
>
> > 0545eec895 meson: Add docs
> >
> > We should think more about how to arrange the documentation.  We
> > probably don't want to copy-and-paste all the introductory and
> > requirements information.  I think we can make this initially much
> > briefer, like the Windows installation chapter.  For example, instead
> > of documenting each setup option again, just mention which ones exist
> > and then point (link) to the configure chapter for details.
>
> The current docs, including the windows ones, are already hard to follow. I
> think we should take some care to not make the meson bits even more
> confusing. Cross referencing left and right seems problematic from that
> angle.
>

On Configure options:

To add to the above, very few sections are an exact copy paste. The
arguments and default behaviors of quite a few configure options are
different. The change in default behavior and arguments is primarily due to
"auto" features which get enabled if the dependencies are found. Whereas
with make, we have explicit --enable or --disable options which don't take
any arguments.

Also, a few instructions / commands which worked with make will need to be
done a bit differently due to environment variables etc. which also had to
be communicated.

Communicating these differences and nuances with cross referencing would
make it confusing as most of this information is in the explanation
paragraph.

On requirements:

They are also a bit different eg. readline is not a "required" thing
anymore, perl, flex, bison are required etc. Also, these are bullet points
with information inlined and not separate sections, so cross-referencing
here also would be hard.

Regards,
Samay


> > I spent a bit of time with the test suites.  I think there is a
> > problem in that selecting a test suite directly, like
> >
> > meson test -C _build --suite recovery
> >
> > doesn't update the tmp_install.  So if this is the first thing you run
> > after a build, everything will fail.  Also, if you run this later, the
> > tmp_install doesn't get updated, so you're not testing up-to-date
> > code.
>
> At the moment creation of the tmp_install is its own test suite. I don't
> know
> if that's the best way, or what the best way is, but that explains that
> fact. You can do the above without the issue by specifying
> --suite setup --suite recovery.
>
>
> Greetings,
>
> Andres Freund
>

Re: Proposal: Support custom authentication methods using hooks

[samay sharma]

Hi Jacob,

On Tue, Aug 2, 2022 at 2:48 PM Jacob Champion 
wrote:

> [CFM hat] Okay, either way you look at it, this patchset needs author
> work before any further review can be done. Peter has given some
> additional feedback on next steps, Stephen has asked for clarification
> on the goals of the patchset, etc. I feel pretty confident in marking
> this Returned with Feedback.
>

That makes sense. I just got back to working on this patch and am aiming to
get this ready for the next commitfest. I plan to address the feedback by
Peter and Jeff and come up with a use case to help clarify the goals to
better answer Stephen's questions.


>
> [dev hat] That said, I plan to do some additional dev work on top of
> this over the next couple of months. The ideal case would be to provide
> a server-only extension that provides additional features to existing
> clients in the wild (i.e. no client-side changes).
>
> I'm also interested in plugging an existing 3rd-party SASL library into
> the client, which would help extensibility on that side.
>

That would be interesting.

Regards,
Samay


>
> --Jacob
>
>

Re: [PoC] Federated Authn/z with OAUTHBEARER

[samay sharma]

Hi Jacob,

Thank you for porting this on top of the pluggable auth methods API. I've
addressed the feedback around other backend changes in my latest patch, but
the client side changes still remain. I had a few questions to understand
them better.

(a) What specifically do the client side changes in the patch implement?
(b) Are the changes you made on the client side specific to OAUTH or are
they about making SASL more generic? As an additional question, if someone
wanted to implement something similar on top of your patch, would they
still have to make client side changes?

Regards,
Samay

On Fri, Mar 4, 2022 at 11:13 AM Jacob Champion  wrote:

> Hi all,
>
> v3 rebases this patchset over the top of Samay's pluggable auth
> provider API [1], included here as patches 0001-3. The final patch in
> the set ports the server implementation from a core feature to a
> contrib module; to switch between the two approaches, simply leave out
> that final patch.
>
> There are still some backend changes that must be made to get this
> working, as pointed out in 0009, and obviously libpq support still
> requires code changes.
>
> --Jacob
>
> [1]
> https://www.postgresql.org/message-id/flat/CAJxrbyxTRn5P8J-p%2BwHLwFahK5y56PhK28VOb55jqMO05Y-DJw%40mail.gmail.com
>

Re: Proposal: Support custom authentication methods using hooks

[samay sharma]

Hi,

On Wed, Mar 16, 2022 at 8:02 AM Stephen Frost  wrote:

> Greetings,
>
> * samay sharma (smilingsa...@gmail.com) wrote:
> > On Fri, Mar 4, 2022 at 11:15 AM Jacob Champion 
> wrote:
> > > On Thu, 2022-03-03 at 11:12 +0100, Peter Eisentraut wrote:
> > > > At the moment, it is not possible to judge whether the hook interface
> > > > you have chosen is appropriate.
> > > >
> > > > I suggest you actually implement the Azure provider, then make the
> hook
> > > > interface, and then show us both and we can see what to do with it.
> > >
> > > To add a data point here, I've rebased my OAUTHBEARER experiment [1] on
> > > top of this patchset. (That should work with Azure's OIDC provider, and
> > > if it doesn't, I'd like to know why.)
> >
> > Firstly, thanks for doing this. It helps to have another data point and
> the
> > feedback you provided is very valuable. I've looked to address it with
> the
> > patchset attached to this email.
> >
> > This patch-set adds the following:
> >
> > * Allow multiple custom auth providers to be registered (Addressing
> > feedback from Aleksander and Andrew)
> > * Modify the test extension to use SCRAM to exchange secrets (Based on
> > Andres's suggestion)
> > * Add support for custom auth options to configure provider's behavior
> (by
> > exposing a new hook) (Required by OAUTHBEARER)
> > * Allow custom auth methods to use usermaps. (Required by OAUTHBEARER)
>
> That we're having to extend this quite a bit to work for the proposed
> OAUTH patches and that it still doesn't do anything for the client side
> (note that the OAUTHBEARER patches are still patching libpq to add
> support directly and surely will still be even with this latest patch
> set ...) makes me seriously doubt that this is the right direction to be
> going in.
>

I think I should have clarified this in my previous email. There is nothing
specific to OAUTHBEARER in these changes. Having auth options and using
usermaps is something which any auth method could require. I had not added
them yet, but I'm pretty sure this would have been requested.


>
> > > After the port, here are the changes I still needed to carry in the
> > > backend to get the tests passing:
> > >
> > > - I needed to add custom HBA options to configure the provider.
> >
> > Could you try to rebase your patch to use the options hook and let me
> know
> > if it satisfies your requirements?
> >
> > Please let me know if there's any other feedback.
>
> How about- if we just added OAUTH support directly into libpq and the
> backend, would that work with Azure's OIDC provider?  If not, why not?
>

Overall, Azure AD implements OIDC, so this could be doable. However, we'll
have to explore further to see if the current implementation would satisfy
all the requirements.


> If it does, then what's the justification for trying to allow custom
> backend-only authentication methods?
>

The only goal of this patch wasn't to enable support for Azure AD. That's
just one client. Users might have a need to add or change auth methods in
the future and providing that extensibility so we don't need to have core
changes for each one of them would be useful. I know there isn't alignment
on this yet, but if we'd like to move certain auth methods out of core into
extensions, then this might provide a good framework for that.

Regards,
Samay


>
> Thanks,
>
> Stephen
>

Re: Proposal: Support custom authentication methods using hooks

[samay sharma]

Hi,

On Fri, Mar 4, 2022 at 11:15 AM Jacob Champion  wrote:

> On Thu, 2022-03-03 at 11:12 +0100, Peter Eisentraut wrote:
> > At the moment, it is not possible to judge whether the hook interface
> > you have chosen is appropriate.
> >
> > I suggest you actually implement the Azure provider, then make the hook
> > interface, and then show us both and we can see what to do with it.
>
> To add a data point here, I've rebased my OAUTHBEARER experiment [1] on
> top of this patchset. (That should work with Azure's OIDC provider, and
> if it doesn't, I'd like to know why.)
>

Firstly, thanks for doing this. It helps to have another data point and the
feedback you provided is very valuable. I've looked to address it with the
patchset attached to this email.

This patch-set adds the following:

* Allow multiple custom auth providers to be registered (Addressing
feedback from Aleksander and Andrew)
* Modify the test extension to use SCRAM to exchange secrets (Based on
Andres's suggestion)
* Add support for custom auth options to configure provider's behavior (by
exposing a new hook) (Required by OAUTHBEARER)
* Allow custom auth methods to use usermaps. (Required by OAUTHBEARER)


> After the port, here are the changes I still needed to carry in the
> backend to get the tests passing:
>
> - I needed to add custom HBA options to configure the provider.
>

Could you try to rebase your patch to use the options hook and let me know
if it satisfies your requirements?

Please let me know if there's any other feedback.

Regards,
Samay


> - I needed to declare usermap support so that my provider could
> actually use check_usermap().

- I had to modify the SASL mechanism registration to allow a custom
> maximum message length, but I think that's not the job of Samay's
> proposal to fix; it's just a needed improvement to CheckSASLAuth().
>
> Obviously, the libpq frontend still needs to understand how to speak
> the new SASL mechanism. There are third-party SASL implementations that
> are plugin-based, which could potentially ease the pain here, at the
> expense of a major dependency and a very new distribution model.
>
> --Jacob
>
> [1]
> https://www.postgresql.org/message-id/flat/d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel%40vmware.com
>


v3-0001-Add-support-for-custom-authentication-methods.patch
Description: Binary data


v3-0004-Add-support-for-map-and-custom-auth-options.patch
Description: Binary data


v3-0002-Add-sample-extension-to-test-custom-auth-provider.patch
Description: Binary data


v3-0003-Add-tests-for-test_auth_provider-extension.patch
Description: Binary data

Re: Proposal: Support custom authentication methods using hooks

[samay sharma]

Hi,

On Wed, Mar 2, 2022 at 12:32 AM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:

> On 01.03.22 22:34, Andres Freund wrote:
> > The cases I've heard about are about centralizing auth across multiple
> cloud
> > services. Including secret management in some form. E.g. allowing an
> > application to auth to postgres, redis and having the secret provided by
> > infrastructure, rather than having to hardcode it in config or such.
> >
> > I can't see application developers configuring kerberos and I don't think
> > LDAP, PAM, Radius are a great answer either, due to the plaintext
> requirement
> > alone? LDAP is pretty clearly dying technology, PAM is fragile
> complicated C
> > stuff that's not portable across OSs. Radius is probably the most
> realistic,
> > but at least as implemented doesn't seem flexible enough (e.g. no access
> to
> > group memberships etc).
> >
> > Nor does baking stuff like that in seem realistic to me, it'll
> presumably be
> > too cloud provider specific.
>
> Let's gather some more information on this.  PostgreSQL should support
> the authentication that many people want to use out of the box.  I don't
> think it would be good to be at a point where all the built-in methods
> are outdated and if you want to use the good stuff you have to hunt for
> plugins.  The number of different cloud APIs is effectively small.  I
> expect that there are a lot of similarities, like they probably all need
> support for http calls, they might need support for caching lookups,
> etc.  OIDC was mentioned elsewhere.  That's a standard.  Is that
> compatible with any cloud providers?  Would that be enough for many users?
>

I think we are discussing two topics in this thread which in my opinion are
orthogonal.


(a) Should we make authentication methods pluggable by exposing these
hooks? - These will allow users to add plugins of their own to support
whatever auth method they like. One immediate use case (and what prompted
me to start looking at this) is Azure Active Directory integration which is
a common request from Azure customers. We could also, over time, move some
of our existing auth methods into extensions if we don’t want to maintain
them in core.


Please note that these hooks do not restrict auth providers to use only
plaintext auth methods. We could do SCRAM with secrets which are stored
outside of Postgres using this auth plugin too. I can modify the
test_auth_provider sample extension to do something like that as Andres
suggested.


I don't see much point in that unless we deprecate *all* the
> auth methods that transmit a cleartext password.


(b) Should we allow plaintext auth methods (and should we deprecate a few
currently supported auth methods which use plaintext exchange)? - I think
this is also a very important discussion but has many factors to consider
independent of the hooks. Whatever decision is made here, we can impose
that in auth.c later for plugins. For eg. allow plugins to only do
plaintext stuff with SSL enabled (by checking if Port->ssl_in_use), or if
we remove AUTH_REQ_PASSWORD, then plugins any way can’t use plaintext
exchange with the client.


So, overall, if we are open to allowing plugins for auth methods, I can
proceed to modify the test_auth_provider extension to use SCRAM and allow
registering multiple providers for this specific change.


Regards,

Samay

Re: Proposal: Support custom authentication methods using hooks

[samay sharma]

Hi Aleksander,

On Thu, Feb 24, 2022 at 1:17 AM Aleksander Alekseev <
aleksan...@timescale.com> wrote:

> Hi Samay,
>
> > I wanted to submit a patch to expose 2 new hooks (one for the
> authentication check and another one for error reporting) in auth.c. These
> will allow users to implement their own authentication methods for Postgres
> or add custom logic around authentication.
>
> I like the idea - PostgreSQL is all about extendability. Also, well
> done with TAP tests and an example extension. This being said, I
> didn't look at the code yet, but cfbot seems to be happy with it:
> http://cfbot.cputube.org/


Thank you!

>
>
> > One constraint in the current implementation is that we allow only one
> authentication provider to be loaded at a time. In the future, we can add
> more functionality to maintain an array of hooks and call the appropriate
> one based on the provider name in the pg_hba line.
>
> This sounds like a pretty severe and unnecessary limitation to me. Do
> you think it would be difficult to bypass it in the first
> implementation?
>

Just to clarify, the limitation is around usage of multiple custom
providers but does allow users to use the existing methods with one custom
authentication method. The reasons I thought this is ok to start with are:
* It allows users to plugin a custom authentication method which they can't
do today.
* Users *generally* have an authentication method for their database (eg.
we use ldap for authentication, or we use md5 passwords for
authentication). While it is possible, I'm not sure how many users use
completely different authentication methods (other than standard ones like
password and trust) for different IPs/databases for their same instance.

So, I thought this should be good enough for a number of use cases.

I thought about adding support for multiple custom providers and the
approach I came up with is: Maintaining a list of all providers (their
names, check functions and error functions), making sure they are all valid
while parsing pg_hba.conf and calling the right one when we see the custom
keyword in pg_hba.conf based on name. I'm not sure (I haven't yet checked)
if we have other such hooks in Postgres where multiple extensions use them
and Postgres calls the right hook based on input (instead of just calling
whoever has the hook).

Therefore, I wanted to start with something simple so users can begin using
auth methods of their choice, and add multiple providers as an enhancement
after doing more research and coming up with the right way to implement it.

That being said, any thoughts on the approach I mentioned above? I'll
look into it and see if it's not too difficult to implement this.

Regards,
Samay


> --
> Best regards,
> Aleksander Alekseev
>

Re: Proposal: Support custom authentication methods using hooks

[samay sharma]

Hi all,

On Thu, Feb 17, 2022 at 11:25 AM samay sharma 
wrote:

> Hi all,
>
> I wanted to submit a patch to expose 2 new hooks (one for the
> authentication check and another one for error reporting) in auth.c. These
> will allow users to implement their own authentication methods for Postgres
> or add custom logic around authentication.
>
> A use case where this is useful are environments where you want
> authentication to be centrally managed across different services. This is a
> common deployment model for cloud providers where customers like to use
> single sign on and authenticate across different services including
> Postgres. Implementing this now is tricky as it requires syncing that
> authentication method's credentials with Postgres (and that gets trickier
> with TTL/expiry etc.). With these hooks, you can implement an extension to
> check credentials directly using the authentication provider's APIs.
>
> To enable this, I've proposed adding a new authentication method "custom"
> which can be specified in pg_hba.conf and takes a mandatory argument
> "provider" specifying which authentication provider to use. I've also moved
> a couple static functions to headers so that extensions can call them.
>
> Sample pg_hba.conf line to use a custom provider:
>
> hostall all ::1/128 custom
> provider=test
>
>
> As an example and a test case, I've added an extension named
> test_auth_provider in src/test/modules which fetches credentials from
> a pre-defined array. I've also added tap tests for the extension to test
> this functionality.
>
>
> One constraint in the current implementation is that we allow only one
> authentication provider to be loaded at a time. In the future, we can add
> more functionality to maintain an array of hooks and call the appropriate
> one based on the provider name in the pg_hba line.
>
>
> A couple of my tests are flaky and sometimes fail in CI. I think the
> reason for that is I don't wait for pg_hba reload to be processed before
> checking logs for error messages. I didn't find an immediate way to address
> that and I'm looking into it but wanted to get an initial version out for
> feedback on the approach taken and interfaces. Once those get finalized, I
> can submit a patch to add docs as well.
>

I wanted to submit a v2 of my patches.

To fix the flaky tests, I decided to avoid checking the log files for
pg_hba reload errors and rely on the output of pg_hba_file_rules. While
doing that, I found two bugs (in my code) which were causing the custom
provider line to not be outputted correctly in pg_hba_file_rules.

This updated patch-set fixes those bugs and also uses pg_hba_file_rules to
check for errors arising due to improper configuration. After these
changes, I've stopped seeing CI failures.

Looking forward to feedback on the overall change and the approach taken.

Regards,
Samay

>
> Looking forward to your feedback.
>
>
> Regards,
>
> Samay
>


v2-0003-Add-tests-for-test_auth_provider-extension.patch
Description: Binary data


v2-0001-Add-support-for-custom-authentication-methods.patch
Description: Binary data


v2-0002-Add-sample-extension-to-test-custom-auth-provider.patch
Description: Binary data

Proposal: Support custom authentication methods using hooks

[samay sharma]

Hi all,

I wanted to submit a patch to expose 2 new hooks (one for the
authentication check and another one for error reporting) in auth.c. These
will allow users to implement their own authentication methods for Postgres
or add custom logic around authentication.

A use case where this is useful are environments where you want
authentication to be centrally managed across different services. This is a
common deployment model for cloud providers where customers like to use
single sign on and authenticate across different services including
Postgres. Implementing this now is tricky as it requires syncing that
authentication method's credentials with Postgres (and that gets trickier
with TTL/expiry etc.). With these hooks, you can implement an extension to
check credentials directly using the authentication provider's APIs.

To enable this, I've proposed adding a new authentication method "custom"
which can be specified in pg_hba.conf and takes a mandatory argument
"provider" specifying which authentication provider to use. I've also moved
a couple static functions to headers so that extensions can call them.

Sample pg_hba.conf line to use a custom provider:

hostall all ::1/128 custom
provider=test


As an example and a test case, I've added an extension named
test_auth_provider in src/test/modules which fetches credentials from
a pre-defined array. I've also added tap tests for the extension to test
this functionality.


One constraint in the current implementation is that we allow only one
authentication provider to be loaded at a time. In the future, we can add
more functionality to maintain an array of hooks and call the appropriate
one based on the provider name in the pg_hba line.


A couple of my tests are flaky and sometimes fail in CI. I think the reason
for that is I don't wait for pg_hba reload to be processed before checking
logs for error messages. I didn't find an immediate way to address that and
I'm looking into it but wanted to get an initial version out for
feedback on the approach taken and interfaces. Once those get finalized, I
can submit a patch to add docs as well.


Looking forward to your feedback.


Regards,

Samay


0001-Add-support-for-custom-authentication-methods.patch
Description: Binary data


0002-Add-sample-extension-to-test-custom-auth-provider-ho.patch
Description: Binary data


0003-Add-tests-for-test_auth_provider-extension.patch
Description: Binary data

Re: Error running configure on Mac

[samay sharma]

On Sun, Jan 23, 2022 at 11:14 PM Tom Lane  wrote:

> samay sharma  writes:
> > I was trying to build Postgres from source on my Mac (MacOS Monterey
> 12.1)
> > and ran into an error when running configure.
>
> Works for me, and for other developers, and for assorted buildfarm
> animals.
>
> > checking for gcc option to accept ISO C99... unsupported
> > configure: error: C compiler "gcc" does not support C99
>
> That is bizarre.  Can you show the segment of config.log
> that corresponds to this?  The exact error message that
> the compiler is reporting would be useful.
>

The line above the error message in config.log is:

configure:4607: result: unsupported

configure:4623: error: C compiler "gcc" does not support C99


Slightly above that, I see this error message too:


configure:4591: gcc -qlanglvl=extc99 -c -g -O2  conftest.c >&5

clang: error: unknown argument: '-qlanglvl=extc99'

configure:4591: $? = 1

configure: failed program was:




I also see many more error messages in config.log when I grep for error.
So, I've attached the entire file in case any other output is useful.



> Also, I wonder if you are using Apple's gcc (yeah, that's
> really clang), or a gcc from MacPorts or Brew or the like.
>

I've pasted the output of gcc --version in the previous email which reports
it to be Apple clang version 13.0.0 (clang-1300.0.27.3). Is there any other
command which I can run to give more info about this?

Regards,
Samay

>
> regards, tom lane
>


config.log
Description: Binary data

Error running configure on Mac

[samay sharma]

Hi,

I was trying to build Postgres from source on my Mac (MacOS Monterey 12.1)
and ran into an error when running configure.

./configure


...

checking for gcc option to accept ISO C99... unsupported

configure: error: C compiler "gcc" does not support C99


When I do gcc --version I see:

Configured with: --prefix=/Library/Developer/CommandLineTools/usr
--with-gxx-include-dir=/Library/Developer/CommandLineTools/SDKs/MacOSX12.1.sdk/usr/include/c++/4.2.1

Apple clang version 13.0.0 (clang-1300.0.27.3)

Target: x86_64-apple-darwin21.2.0

Thread model: posix

InstalledDir: /Library/Developer/CommandLineTools/usr/bin


So, it looks like it is using clang to compile and not gcc.


configure runs successfully if I do: ./configure CC=gcc-11 or soft link gcc
to gcc-11 and then run configure. However, I didn't find these tips in
Platform specific notes in docs:
https://www.postgresql.org/docs/9.6/installation-platform-notes.html#INSTALLATION-NOTES-MACOS
.


So, I wanted to ask if this behavior is expected and if so, should we
update docs to make a note of this?


Regards,

Samay

Re: New developer papercut - Makefile references INSTALL

[samay sharma]

On Wed, Jan 19, 2022 at 4:58 PM Tim McNamara  wrote:

> Hello,
>
> I encountered a minor road bump when checking out the pg source today. The
> Makefile's all target includes the following help message if GNUmakefile
> isn't available:
>
>   echo "You need to run the 'configure' program first. See the file"; \
>   echo "'INSTALL' for installation instructions." ; \
>
> After consulting README.git, it looks as though INSTALL isn't created
> unless the source is bundled into a release or snapshot tarball. I'm happy
> to submit a patch to update the wording, but wanted to check on the
> preferred approach.
>
> Perhaps this would be sufficient?
>
>   echo "You need to run the 'configure' program first. See the file"; \
>   echo "'INSTALL' for installation instructions, or visit" ; \
>   echo "" ; \
>
> -Tim
>

I noticed a similar thing in the README of the github repository. It asks
to see the INSTALL file for build and installation instructions but I
couldn't find that file and that confused me. This might confuse other new
developers as well. So, maybe we should update the text in the README too?

Regards,
Samay