Re: MAINTAIN privilege -- what do we need to un-revert it?

[Yugo Nagata]

On Fri, 2 Aug 2024 16:13:01 +0900
Yugo NAGATA  wrote:

> On Thu, 01 Aug 2024 11:31:53 -0700
> Jeff Davis  wrote:
> 
> > On Wed, 2024-07-31 at 18:20 +0900, Yugo NAGATA wrote:
> > > I agree that it might not be important, but I think adding the flag
> > > would be
> > > also helpful for improving code-readability because it clarify the
> > > function
> > > is used in the two cases. I attached patch for this fix (patch 0003).
> > 
> > Committed with one minor modification: I moved the boolean flag to be
> > near the other booleans rather than at the end. Thank you.
> > 
> > > Sure. I fixed the patch to remove 'param' from both functions. (patch
> > > 0002)
> > 
> > Committed, thank you.
> 
> Thank you for committing them.
> Should not they be backported to REL_17_STABLE?
> 
> > 
> > > I also add the small refactoring around ExecCreateTableAs(). (patch
> > > 0001)
> > > 
> > > - Remove matview-related codes from intorel_startup.
> > >   Materialized views are no longer handled in this function.
> > > 
> > > - RefreshMatViewByOid is moved to just after create_ctas_nodata
> > >   call to improve code readability.
> > > 
> > 
> > I'm not sure the changes in intorel_startup() are correct. I tried
> > adding an Assert(into->viewQuery == NULL), and it fails because there's
> > another path I did not consider: "EXPLAIN ANALYZE CREATE MATERIALIZED
> > VIEW ...", which does not go through ExecCreateTableAs() but does go
> > through CreateIntoRelDestReceiver().
> > 
> > See:
> > 
> > https://postgr.es/m/20444c382e6cb5e21e93c94d679d0198b0dba4dd.ca...@j-davis.com
> > 
> > Should we refactor a bit and try to make EXPLAIN use the same code
> > paths?
> 
> I overlooked that CreateIntoRelDestReceiver() is used from EXPLAIN. I saw the
> thread above and I agree that we should refactor it to make EXPLAIN consistent
> CREATE MATERIALIZED VIEW, but I suppose this should be discussed the other 
> thread.
> 
> I attached a updated patch removed the intorel_startup() part from.

I confirmed that this has been committed to the master branch.
Thank you!

I also noticed that the documentation of CREATE MATERIALIZED VIEW doesn't 
mention
search_path while it also changes search_path since it uses the REFRESH logic.
I attached a trivial patch to fix this.

Regards,
Yugo Nagata


-- 
Yugo Nagata 
diff --git a/doc/src/sgml/ref/create_materialized_view.sgml b/doc/src/sgml/ref/create_materialized_view.sgml
index 0d2fea2b97..1eb27166d9 100644
--- a/doc/src/sgml/ref/create_materialized_view.sgml
+++ b/doc/src/sgml/ref/create_materialized_view.sgml
@@ -163,6 +163,16 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] table_name
   
  
 
+ 
+  Notes
+
+  
+   While CREATE MATERIALIZED VIEW is running, the  is temporarily changed to pg_catalog,
+   pg_temp.
+  
+ 
+
  
   Compatibility
 

Re: Set query_id for query contained in utility statement

[Anthonin Bonnefoy]

I've realised my initial approach was wrong, calling the post parse
for all nested queries in analyze.c prevents extension like pgss to
correctly track the query's nesting level.

I've changed the approach to replicate what's done in ExplainQuery to
both CreateTableAs and DeclareCursor: Jumble the query contained by
the utility statement and call the post parse hook before it is
planned or executed. Additionally, explain's nested query can itself
be a CreateTableAs or DeclareCursor which also needs to be jumbled.
The issue is visible when explaining a CreateTableAs or DeclareCursor
Query, the queryId is missing despite the verbose.

EXPLAIN (verbose) create table test_t as select 1;
QUERY PLAN
--
 Result  (cost=0.00..0.01 rows=1 width=4)
   Output: 1

Post patch, the query id is correctly displayed.

EXPLAIN (verbose) create table test_t as select 1;
QUERY PLAN
--
 Result  (cost=0.00..0.01 rows=1 width=4)
   Output: 1
 Query Identifier: 2800308901962295548

Regards,
Anthonin Bonnefoy


v2-0001-Set-query_id-for-queries-contained-in-utility-sta.patch
Description: Binary data

Re: Remove support for old realpath() API

[Heikki Linnakangas]

On 05/08/2024 09:12, Peter Eisentraut wrote:

The now preferred way to call realpath() is by passing NULL as the
second argument and get a malloc'ed result.  We still supported the
old way of providing our own buffer as a second argument, for some
platforms that didn't support the new way yet.  Those were only
Solaris less than version 11 and some older AIX versions (7.1 and
newer appear to support the new variant).  We don't support those
platforms versions anymore, so we can remove this extra code.


+1

We don't seem to have any mentions of POSIX or SuS in docs, in the 
installation sections. There are a few mentions of POSIX-1.2008 and 
POSIX-1.2001 it in the commit log, though, where we require features 
specified by those. Can we rely on everything from POSIX-1-2008 
nowadays, or is it more on a case-by-case basis, depending on which 
parts of POSIX are supported by various platforms?


--
Heikki Linnakangas
Neon (https://neon.tech)

Re: Fixed small typo in bufpage.h

[Amit Kapila]

On Mon, Aug 5, 2024 at 6:54 AM Senglee Choi  wrote:
>
> Fixed a minor typo in src/include/storage/bufpage.h.
>

LGTM. BTW, we do use two spaces before the new sentence. See comments
in the nearby code.

-- 
With Regards,
Amit Kapila.

Re: Logical Replication of sequences

[vignesh C]

On Fri, 2 Aug 2024 at 14:24, shveta malik  wrote:
>
> On Thu, Aug 1, 2024 at 9:26 AM shveta malik  wrote:
> >
> > On Mon, Jul 29, 2024 at 4:17 PM vignesh C  wrote:
> > >
> > > Thanks for reporting this, these issues are fixed in the attached
> > > v20240730_2 version patch.
> > >
>
> I was reviewing the design of patch003, and I have a query. Do we need
> to even start an apply worker and create replication slot when
> subscription created is for 'sequences only'? IIUC, currently logical
> replication apply worker is the one launching sequence-sync worker
> whenever needed. I think it should be the launcher doing this job and
> thus apply worker may even not be needed for current functionality of
> sequence sync? Going forward when we implement incremental sync of
> sequences, then we may have apply worker started but now it is not
> needed.

I believe the current method of having the apply worker initiate the
sequence sync worker is advantageous for several reasons:
a) Reduces Launcher Load: This approach prevents overloading the
launcher, which must handle various other subscription requests.
b) Facilitates Incremental Sync: It provides a more straightforward
path to extend support for incremental sequence synchronization.
c) Reuses Existing Code: It leverages the existing tablesync worker
code for starting the tablesync process, avoiding the need to
duplicate code in the launcher.
d) Simplified Code Maintenance: Centralizing sequence synchronization
logic within the apply worker can simplify code maintenance and
updates, as changes will only need to be made in one place rather than
across multiple components.
e) Better Monitoring and Debugging: With sequence synchronization
being handled by the apply worker, you can more effectively monitor
and debug synchronization processes since all related operations are
managed by a single component.

Also, I noticed that even when a publication has no tables, we create
replication slot and start apply worker.

Regards,
Vignesh

Re: Logical Replication of sequences

[vignesh C]

On Fri, 2 Aug 2024 at 14:33, shveta malik  wrote:
>
> On Fri, Aug 2, 2024 at 2:24 PM shveta malik  wrote:
> >
> > On Thu, Aug 1, 2024 at 9:26 AM shveta malik  wrote:
> > >
> > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C  wrote:
> > > >
> > > > Thanks for reporting this, these issues are fixed in the attached
> > > > v20240730_2 version patch.
> > > >
> >
> > I was reviewing the design of patch003, and I have a query. Do we need
> > to even start an apply worker and create replication slot when
> > subscription created is for 'sequences only'? IIUC, currently logical
> > replication apply worker is the one launching sequence-sync worker
> > whenever needed. I think it should be the launcher doing this job and
> > thus apply worker may even not be needed for current functionality of
> > sequence sync? Going forward when we implement incremental sync of
> > sequences, then we may have apply worker started but now it is not
> > needed.
> >
>
> Also, can we please mention the state change and 'who does what' atop
> sequencesync.c file similar to what we have atop tablesync.c file
> otherwise it is difficult to figure out the flow.

I have added this in sequencesync.c file, the changes for the same are
available at v20240805_2 version patch at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm1kk1MHGk3BU_XTxay%3DdR6sMHnm4TT5cmVz2f_JXkWENQ%40mail.gmail.com

Regards,
Vignesh

Re: [BUG?] check_exclusion_or_unique_constraint false negative

[Amit Kapila]

On Fri, Aug 2, 2024 at 10:38 PM Michail Nikolaev
 wrote:
>
> > I think it is rather less likely or not possible in a parallel apply
> > case because such conflicting updates (updates on the same tuple)
> > should be serialized at the publisher itself. So one of the updates
> > will be after the commit that has the second update.
>
> Glad to hear! But anyway, such logic looks very fragile to me.
>
> > I haven't tried the test based on your description of the general
> > problem with DirtySnapshot scan. In case of logical replication, we
> > will LOG update_missing type of conflict and the user may need to take
> > some manual action based on that.
>
> Current it is just DEBUG1, so it will be probably missed by the user.
>
> > * XXX should this be promoted to ereport(LOG) perhaps?
> > */
> > elog(DEBUG1,
> > "logical replication did not find row to be updated "
> > "in replication target relation \"%s\"",
> > RelationGetRelationName(localrel));
> > }
>

Right, but we are extending this functionality to detect and resolve
such conflicts [1][2]. I am hoping after that such updates won't be
missed.

[1] - https://commitfest.postgresql.org/49/5064/
[2] - https://commitfest.postgresql.org/49/5021/


-- 
With Regards,
Amit Kapila.

Re: Conflict detection and logging in logical replication

[Amit Kapila]

On Sun, Aug 4, 2024 at 1:22 PM Zhijie Hou (Fujitsu)
 wrote:
>
> On Friday, August 2, 2024 7:03 PM Amit Kapila  wrote:
> >
>
> Here is the V11 patch set which addressed above and Kuroda-san[1] comments.
>

A few design-level points:

*
@@ -525,10 +602,33 @@ ExecSimpleRelationInsert(ResultRelInfo *resultRelInfo,
  /* OK, store the tuple and create index entries for it */
  simple_table_tuple_insert(resultRelInfo->ri_RelationDesc, slot);

+ conflictindexes = resultRelInfo->ri_onConflictArbiterIndexes;
+
  if (resultRelInfo->ri_NumIndices > 0)
  recheckIndexes = ExecInsertIndexTuples(resultRelInfo,
-slot, estate, false, false,
-NULL, NIL, false);
+slot, estate, false,
+conflictindexes ? true : false,
+&conflict,
+conflictindexes, false);
+
+ /*
+ * Checks the conflict indexes to fetch the conflicting local tuple
+ * and reports the conflict. We perform this check here, instead of
+ * performing an additional index scan before the actual insertion and
+ * reporting the conflict if any conflicting tuples are found. This is
+ * to avoid the overhead of executing the extra scan for each INSERT
+ * operation, even when no conflict arises, which could introduce
+ * significant overhead to replication, particularly in cases where
+ * conflicts are rare.
+ *
+ * XXX OTOH, this could lead to clean-up effort for dead tuples added
+ * in heap and index in case of conflicts. But as conflicts shouldn't
+ * be a frequent thing so we preferred to save the performance overhead
+ * of extra scan before each insertion.
+ */
+ if (conflict)
+ CheckAndReportConflict(resultRelInfo, estate, CT_INSERT_EXISTS,
+recheckIndexes, slot);

I was thinking about this case where we have some pros and cons of
doing additional scans only after we found the conflict. I was
wondering how we will handle the resolution strategy for this when we
have to remote_apply the tuple for insert_exists/update_exists cases.
We would have already inserted the remote tuple in the heap and index
before we found the conflict which means we have to roll back that
change and then start a forest transaction to perform remote_apply
which probably has to update the existing tuple. We may have to
perform something like speculative insertion and then abort it. That
doesn't sound cheap either. Do you have any better ideas?

*
-ERROR:  duplicate key value violates unique constraint "test_pkey"
-DETAIL:  Key (c)=(1) already exists.
+ERROR:  conflict insert_exists detected on relation "public.test"
+DETAIL:  Key (c)=(1) already exists in unique index "t_pkey", which
was modified locally in transaction 740 at 2024-06-26
10:47:04.727375+08.

I think the format to display conflicts is not very clear. The
conflict should be apparent just by seeing the LOG/ERROR message. I am
thinking of something like below:

LOG: CONFLICT: ;
DESCRIPTION: If any .. ; RESOLUTION: (This one can be added later)
DEATAIL: remote_tuple (tuple values); local_tuple (tuple values);

With the above, one can easily identify the conflict's reason and
action taken by apply worker.

-- 
With Regards,
Amit Kapila.

Re: Detailed release notes

[jian he]

On Fri, Jul 26, 2024 at 10:11 PM Tom Lane  wrote:
>
> Daniel Gustafsson  writes:
> >> On 26 Jul 2024, at 15:00, Marcos Pegoraro  wrote:
> >> But why is that just a hidden comment and not a visible link for us ?
>
> > That's likely the wrong level of detail for the overwhelming majority of
> > release notes readers.  I have a feeling this was discussed not too long ago
> > but (if so) I fail to find that discussion now.
>
> Yeah, I too recall some discussion of surfacing the commit links
> somehow, perhaps as a popup tooltip.  Nobody's got round to it yet.
> It's not real clear how to handle multiple links per , which
> happens from time to time in major release notes and just about
> everyplace in minor release notes.
>

similar to https://docs.python.org/3/whatsnew/changelog.html

Change functions to use a safe search_path during maintenance
operations (Jeff Davis)

change to

[commitId_link1, commitId_link2]: Change functions to use a safe
search_path during maintenance operations (Jeff Davis)

Does this make sense?
If so, then we can hardcode and some automation can change to that way.

Re: [BUG?] check_exclusion_or_unique_constraint false negative

[Michail Nikolaev]

Hello!

> Right, but we are extending this functionality to detect and resolve
> such conflicts [1][2]. I am hoping after that such updates won't be
> missed.

Yes, this is a nice feature. However, without the DirtySnapshot index scan
fix, it will fail in numerous instances, especially in master-master
replication.

The update_missing feature is helpful in this case, but it is still not the
correct event because a real tuple exists, and we should receive
update_differ instead. As a result, some conflict resolution systems may
malfunction. For example, if the resolution method is set to apply_or_skip,
it will insert the new row, causing two rows to exist. This system is quite
fragile, and I am sure there are many more complicated scenarios that could
arise.
Best regards,
Mikhail.

Re: Detailed release notes

[Marcos Pegoraro]

Em seg., 5 de ago. de 2024 às 07:54, jian he 
escreveu:

>
> [commitId_link1, commitId_link2]: Change functions to use a safe
> search_path during maintenance operations (Jeff Davis)
>

I don't like that prefix dirtying each item.
I think having just a link after every item would be better.
Firstly because in English we read left to right and
also because we don't need the commit code. So it would be

   - Change functions to use a safe search_path during maintenance
   operations (Jeff Davis) [link1, link2]

or just a number to it

   - Change functions to use a safe search_path during maintenance
   operations (Jeff Davis) [1, 2]


regards
Marcos

Re: Speed up JSON escape processing with SIMD plus other optimisations

[David Rowley]

On Sun, 4 Aug 2024 at 02:11, David Rowley  wrote:
> I did some more testing on this on a few different machines;  apple M2
> Ultra, AMD 7945HX and with a Raspberry Pi 4.

I did some more testing on this patch today as I wanted to see what
Intel CPUs thought about it.  The only modern Intel CPU I have is a
13th-generation laptop CPU. It's an i7-1370P.  It's in a laptop with
solid-state cooling. At least, I've never heard a fan running on it.
Watching the clock speed during the test had it jumping around wildly,
so I assume it was thermally throttling.

I've attached the results here anyway. They're very noisy.

I also did a test where I removed all the escaping logic and had the
code copy the source string to the destination without checking for
chars to escape. I wanted to see how much was left performance-wise.
There was only a further 10% increase.

I tidied up the patch a bit more and pushed it.

Thanks for the reviews.

David

Re: scalability bottlenecks with (many) partitions (and more)

[Tomas Vondra]

Hi,

On 6/25/24 12:04, Tomas Vondra wrote:



On 6/24/24 17:05, Robert Haas wrote:

On Sun, Jan 28, 2024 at 4:57 PM Tomas Vondra
 wrote:

For NUM_LOCK_PARTITIONS this is pretty simple (see 0001 patch). The
LWLock table has 16 partitions by default - it's quite possible that on
machine with many cores and/or many partitions, we can easily hit this.
So I bumped this 4x to 64 partitions.


I think this probably makes sense. I'm a little worried that we're
just kicking the can down the road here where maybe we should be
solving the problem in some more fundamental way, and I'm also a
little worried that we might be reducing single-core performance. But
it's probably fine.



Yeah, I haven't seen this causing any regressions - the sensitive paths
typically lock only one partition, so having more of them does not
affect that. Or if it does, it's likely a reasonable trade off as it
reduces the risk of lock contention.

That being said, I don't recall benchmarking this patch in isolation,
only with the other patches. Maybe I should do that ...


What I ended up doing is having a hash table of 16-element arrays. There
are 64 "pieces", each essentially the (16 x OID + UINT64 bitmap) that we
have now. Each OID is mapped to exactly one of these parts as if in a
hash table, and in each of those 16-element parts we do exactly the same
thing we do now (linear search, removal, etc.). This works great, the
locality is great, etc. The one disadvantage is this makes PGPROC
larger, but I did a lot of benchmarks and I haven't seen any regression
that I could attribute to this. (More about this later.)


I think this is a reasonable approach. Some comments:

- FastPathLocalUseInitialized seems unnecessary to me; the contents of
an uninitialized local variable are undefined, but an uninitialized
global variable always starts out zeroed.



OK. I didn't realize global variables start a zero.



I haven't fixed this yet, but it's pretty clear the "init" is not really 
needed, because it did the memset() wrong:


memset(FastPathLocalUseCounts, 0, sizeof(FastPathLocalUseInitialized));

This only resets one byte of the array, yet it still worked correctly.


- You need comments in various places, including here, where someone
is certain to have questions about the algorithm and choice of
constants:

+#define FAST_PATH_LOCK_REL_GROUP(rel) (((uint64) (rel) * 7883 + 4481)
% FP_LOCK_GROUPS_PER_BACKEND)



Yeah, definitely needs comment explaining this.

I admit those numbers are pretty arbitrary primes, to implement a
trivial hash function. That was good enough for a PoC patch, but maybe
for a "proper" version this should use a better hash function. It needs
to be fast, and maybe it doesn't matter that much if it's not perfect.


When I originally coded up the fast-path locking stuff, I supposed
that we couldn't make the number of slots too big because the
algorithm requires a linear search of the whole array. But with this
one trick (a partially-associative cache), there's no real reason that
I can think of why you can't make the number of slots almost
arbitrarily large. At some point you're going to use too much memory,
and probably before that point you're going to make the cache big
enough that it doesn't fit in the CPU cache of an individual core, at
which point possibly it will stop working as well. But honestly ... I
don't quite see why this approach couldn't be scaled quite far.



I don't think I've heard the term "partially-associative cache" before,
but now that I look at the approach again, it very much reminds me how
set-associative caches work (e.g. with cachelines in CPU caches). It's a
16-way associative cache, assigning each entry into one of 16 slots.

I must have been reading some papers in this area shortly before the PoC
patch, and the idea came from there, probably. Which is good, because it
means it's a well-understood and widely-used approach.


Like, if we raised FP_LOCK_GROUPS_PER_BACKEND from your proposed value
of 64 to say 65536, would that still perform well? I'm not saying we
should do that, because that's probably a silly amount of memory to
use for this, but the point is that when you start to have enough
partitions that you run out of lock slots, performance is going to
degrade, so you can imagine wanting to try to have enough lock groups
to make that unlikely. Which leads me to wonder if there's any
particular number of lock groups that is clearly "too many" or whether
it's just about how much memory we want to use.



That's an excellent question. I don't know.

I agree 64 groups is pretty arbitrary, and having 1024 may not be enough
even with a modest number of partitions. When I was thinking about using
a higher value, my main concern was that it'd made the PGPROC entry
larger. Each "fast-path" group is ~72B, so 64 groups is ~4.5kB, and that
felt like quite a bit.

But maybe it's fine and we could make it much larger - L3 caches tend to
be many MBs these days, although AFAIK it's shared by threads r

Re: Adding OLD/NEW support to RETURNING

[jian he]

On Fri, Aug 2, 2024 at 6:13 PM Dean Rasheed  wrote:
>
> On Fri, 2 Aug 2024 at 08:25, jian he  wrote:
> >
> > if (resultRelInfo->ri_projectReturning && (processReturning || saveOld))
> > {
> > }
> >
> > "saveOld" imply "resultRelInfo->ri_projectReturning"
> > we can simplified it as
> >
> > if (processReturning || saveOld))
> > {
> > }
> >
>
> No, because processReturning can be true when
> resultRelInfo->ri_projectReturning is NULL (no RETURNING list). So we
> do still need to check that resultRelInfo->ri_projectReturning is
> non-NULL.
>
> > for projectReturning->pi_state.flags,
> > we don't use EEO_FLAG_OLD_IS_NULL, EEO_FLAG_NEW_IS_NULL
> > in ExecProcessReturning, we can do the following way.
> >
> > /* Make old/new tuples available to ExecProject, if required */
> > if (oldSlot)
> > econtext->ecxt_oldtuple = oldSlot;
> > else if (projectReturning->pi_state.flags & EEO_FLAG_HAS_OLD)
> > econtext->ecxt_oldtuple = ExecGetAllNullSlot(estate, resultRelInfo);
> > else
> > econtext->ecxt_oldtuple = NULL; /* No references to OLD columns */
> >
> > if (newSlot)
> > econtext->ecxt_newtuple = newSlot;
> > else if (projectReturning->pi_state.flags & EEO_FLAG_HAS_NEW)
> > econtext->ecxt_newtuple = ExecGetAllNullSlot(estate, resultRelInfo);
> > else
> > econtext->ecxt_newtuple = NULL; /* No references to NEW columns */
> >
> > /*
> >  * Tell ExecProject whether or not the OLD/NEW rows exist (needed for 
> > any
> >  * ReturningExpr nodes).
> >  */
> > if (oldSlot == NULL)
> > projectReturning->pi_state.flags |= EEO_FLAG_OLD_IS_NULL;
> > else
> > projectReturning->pi_state.flags &= ~EEO_FLAG_OLD_IS_NULL;
> >
> > if (newSlot == NULL)
> > projectReturning->pi_state.flags |= EEO_FLAG_NEW_IS_NULL;
> > else
> > projectReturning->pi_state.flags &= ~EEO_FLAG_NEW_IS_NULL;
> >
>
> I'm not sure I understand your point. It's true that
> EEO_FLAG_OLD_IS_NULL and EEO_FLAG_NEW_IS_NULL aren't used directly in
> ExecProcessReturning(), but they are used in stuff called from
> ExecProject().
>
> If the point was just to swap those 2 code blocks round, then OK, I
> guess maybe it reads a little better that way round, though it doesn't
> really make any difference either way.

sorry for confusion. I mean "swap those 2 code blocks round".
I think it will make it more readable, because you first check
projectReturning->pi_state.flags
with EEO_FLAG_HAS_NEW, EEO_FLAG_HAS_OLD
then change it.


> I did notice that that comment should mention that ExecEvalSysVar()
> also uses these flags, so I've updated it to do so.
>
> > @@ -2620,6 +2620,13 @@ transformWholeRowRef(ParseState *pstate,
> >   * point, there seems no harm in expanding it now rather than during
> >   * planning.
> >   *
> > + * Note that if the nsitem is an OLD/NEW alias for the target RTE (as can
> > + * appear in a RETURNING list), its alias won't match the target RTE's
> > + * alias, but we still want to make a whole-row Var here rather than a
> > + * RowExpr, for consistency with direct references to the target RTE, and
> > + * so that any dropped columns are handled correctly.  Thus we also check
> > + * p_returning_type here.
> > + *
> > makeWholeRowVar and subroutines only related to pg_type, but dropped
> > column info is in pg_attribute.
> > I don't understand "so that any dropped columns are handled correctly".
> >
>
> The nsitem contains references to dropped columns, so if you expanded
> it as a RowExpr, you'd end up with mismatched columns and it would
> fail (somewhere under ParseFuncOrColumn(), from transformColumnRef(),
> I think). There's a regression test case in returning.sql that covers
> that.
play around with it, get it.

if (nsitem->p_names == nsitem->p_rte->eref ||
nsitem->p_returning_type != VAR_RETURNING_DEFAULT)
else
{
expandRTE(nsitem->p_rte, nsitem->p_rtindex, sublevels_up,
  nsitem->p_returning_type, location, false, NULL, &fields);
}
The ELSE  branch expandRTE include_dropped argument is false.
that makes the ELSE  branch unable to deal with dropped columns.



took me a while to understand the changes in rewriteHandler.c, rewriteManip.c
rule over updateable view still works, but I didn't check closely with
rewriteRuleAction.
i think I understand rewriteTargetView and subroutines.

 * In addition, the caller must provide result_relation, the index of the
 * target relation for an INSERT/UPDATE/DELETE/MERGE.  This is needed to
 * handle any OLD/NEW RETURNING list Vars referencing target_varno.  When such
 * Vars are expanded, varreturningtype is copied onto any replacement Vars
 * that reference result_relation.  In addition, if the replacement expression
 * from the targetlist is not simply a Var referencing result_relation, we
 * wrap it in a ReturningExpr node, to force it to be NULL if the OLD/NEW row
 * doesn't exist.

"the index of the target relation

Re: Is *fast* 32-bit support still important?

[Joel Jacobson]

On Tue, Jul 30, 2024, at 09:25, Heikki Linnakangas wrote:
> On 29/07/2024 23:40, Joel Jacobson wrote:
>> To me, it's non-obvious whether introducing `#if SIZEOF_DATUM < 8` with
>> separate 32-bit and 64-bit code paths is worthwhile to maintain performance
>> for both.
>> 
>> Knowing more about $subject can hopefully help us reason about how much
>> additional code complication is justifiable for *fast* 32-bit support.
>
> IMO I don't think it's worth adding extra code for fast 32-bit support 
> anymore. However, I'd still be wary of *regressing* performance on 
> 32-bit systems.
>
> So if you're adding a new fast path to a function, it's OK to make it 
> 64-bit only, and fall back to the old slower code on 32-bit systems. But 
> -1 on *removing* existing 32-bit fast path code, or rewriting things in 
> a way that makes an existing function significantly slower than before 
> on 32-bit systems.
>
> This isn't black or white though. It depends on how big a gain or 
> regression we're talking about, and how complex the extra code would be.

Thanks for input.

I still haven't got any reports from real users of 32-bit PostgreSQL,
so my comments below are based on the assumption that such users exist
and have high performance needs.

I agree that it's not a black or white decision since quantifying complexity
is inherently challenging.

However, perhaps it would be possible to say something about
lower and upper bounds on 32-bit slowdown?

- Below a certain percentage slowdown,
  extra 32-bit code optimization is definitively unnecessary.

- Above a certain percentage slowdown,
  extra 32-bit code optimization is definitively necessary.

In the range between these bounds, I guess the decision should depend on
the specific added code complexity required?

It's also the question what percentage we're reasoning about here.
Is it the time spent in the function, or is it the total execution time?

/Joel

Re: Logical Replication of sequences

[Amit Kapila]

On Mon, Aug 5, 2024 at 2:36 PM vignesh C  wrote:
>
> On Fri, 2 Aug 2024 at 14:24, shveta malik  wrote:
> >
> > On Thu, Aug 1, 2024 at 9:26 AM shveta malik  wrote:
> > >
> > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C  wrote:
> > > >
> > > > Thanks for reporting this, these issues are fixed in the attached
> > > > v20240730_2 version patch.
> > > >
> >
> > I was reviewing the design of patch003, and I have a query. Do we need
> > to even start an apply worker and create replication slot when
> > subscription created is for 'sequences only'? IIUC, currently logical
> > replication apply worker is the one launching sequence-sync worker
> > whenever needed. I think it should be the launcher doing this job and
> > thus apply worker may even not be needed for current functionality of
> > sequence sync?
>

But that would lead to maintaining all sequence-sync of each
subscription by launcher. Say there are 100 sequences per subscription
and some of them from each subscription are failing due to some
reasons then the launcher will be responsible for ensuring all the
sequences are synced. I think it would be better to handle
per-subscription work by the apply worker.

>
> Going forward when we implement incremental sync of
> > sequences, then we may have apply worker started but now it is not
> > needed.
>
> I believe the current method of having the apply worker initiate the
> sequence sync worker is advantageous for several reasons:
> a) Reduces Launcher Load: This approach prevents overloading the
> launcher, which must handle various other subscription requests.
> b) Facilitates Incremental Sync: It provides a more straightforward
> path to extend support for incremental sequence synchronization.
> c) Reuses Existing Code: It leverages the existing tablesync worker
> code for starting the tablesync process, avoiding the need to
> duplicate code in the launcher.
> d) Simplified Code Maintenance: Centralizing sequence synchronization
> logic within the apply worker can simplify code maintenance and
> updates, as changes will only need to be made in one place rather than
> across multiple components.
> e) Better Monitoring and Debugging: With sequence synchronization
> being handled by the apply worker, you can more effectively monitor
> and debug synchronization processes since all related operations are
> managed by a single component.
>
> Also, I noticed that even when a publication has no tables, we create
> replication slot and start apply worker.
>

As far as I understand slots and origins are primarily required for
incremental sync. Would it be used only for sequence-sync cases? If
not then we can avoid creating those. I agree that it would add some
complexity to the code with sequence-specific checks, so we can create
a top-up patch for this if required and evaluate its complexity versus
the benefit it produces.

-- 
With Regards,
Amit Kapila.

Re: Is *fast* 32-bit support still important?

[Joel Jacobson]

On Tue, Jul 30, 2024, at 11:06, Aleksander Alekseev wrote:
> Hi Joel,
>
> Here are my two cents.
>
>> 1. Who are the current users of 32-bit PostgreSQL?
>
> Pretty much any embedded system that uses just a few GB of memory may
> win from using a 32-bit processor (not necessarily in terms of
> performance, maybe in terms of price). Think of WiFi-routers, smart
> TVs, 3D printers, etc.

Thanks for feedback!

Do we know of any such products or users?

I found an OS that runs on many 32-bit chips, FreeRTOS, that seems quite 
popular.
Couldn't find anything about PostgreSQL and FreeRTOS though.
I've posted a question on their forum. [1] Let's wait and see if we hear from 
any real user.

I see one i386 and i686 build farm animals runs Debian.
Perhaps it makes sense to try to reach out to the Debian community,
and see if they know of any PostgreSQL users on 32-bit?

> Generally speaking it's hard to give an exact answer due to lack of
> "telemetry" in PostgreSQL.

Could we add a text message that is displayed to a user,
when compiling PostgreSQL on a 32-bit platform?

*
NOTICE: You are compiling PostgreSQL on a 32-bit platform.

We are interested in learning about your use case.
Please contact us with details about how you are using
PostgreSQL on this platform.

Thank you!

Contact: pgsql-hack...@postgresql.org
Subject: Report from 32-bit user
*

/Joel

Re: Is *fast* 32-bit support still important?

[Aleksander Alekseev]

Hi,

> > Pretty much any embedded system that uses just a few GB of memory may
> > win from using a 32-bit processor (not necessarily in terms of
> > performance, maybe in terms of price). Think of WiFi-routers, smart
> > TVs, 3D printers, etc.
>
> Thanks for feedback!
>
> Do we know of any such products or users?
>
> I found an OS that runs on many 32-bit chips, FreeRTOS, that seems quite 
> popular.
> Couldn't find anything about PostgreSQL and FreeRTOS though.
> I've posted a question on their forum. [1] Let's wait and see if we hear from 
> any real user.

I'm not extremely familiar with FreeRTOS but my humble understanding
is that it's very different from what we would typically call an
"operating system". If I'm not wrong, basically this is a framework
that adds multitasking to STM32 microcontrollers. This is not exactly
a target hardware for PostgreSQL (although running PostgreSQL on STM32
MCUs could be a fun project for someone looking for a challenging
task.)

> I see one i386 and i686 build farm animals runs Debian.
> Perhaps it makes sense to try to reach out to the Debian community,
> and see if they know of any PostgreSQL users on 32-bit?
>
> > Generally speaking it's hard to give an exact answer due to lack of
> > "telemetry" in PostgreSQL.
>
> Could we add a text message that is displayed to a user,
> when compiling PostgreSQL on a 32-bit platform?

What would be actionable items depending on the results? Option A:
someone claims to use PostgreSQL on 32-bit hardware. Option B: no one
admits that they use PostgreSQL on 32-bit hardware (but maybe someone
actually does and/or will in the future). Regardless of the results
you can't drop the support of 32-bit software (until it gets as
difficult and pointless as with AIX that was dropped recently) and it
will not tell you how slow the 32-bit version of PostgreSQL can be.

If there are no actionable items why create a poll?

-- 
Best regards,
Aleksander Alekseev

Re: Optimize mul_var() for var1ndigits >= 8

[Joel Jacobson]

On Tue, Jul 30, 2024, at 00:31, Tom Lane wrote:
> Dean Rasheed  writes:
>> On Mon, 29 Jul 2024 at 21:39, Joel Jacobson  wrote:
>>> I think it's non-obvious if the separate code paths for 32-bit and 64-bit,
>>> using `#if SIZEOF_DATUM < 8`, to get *fast* 32-bit support, outweighs
>>> the benefits of simpler code.
>
>> Looking at that other thread that you found [1], I think it's entirely
>> possible that there are people who care about 32-bit systems, which
>> means that we might well get complaints, if we make it slower for
>> them. Unfortunately, I don't have any way to test that (I doubt that
>> running a 32-bit executable on my x86-64 system is a realistic test).
>
> I think we've already done things that might impact 32-bit systems
> negatively (5e1f3b9eb for instance), and not heard a lot of pushback.
> I would argue that anyone still running PG on 32-bit must have pretty
> minimal performance requirements, so that they're unlikely to care if
> numeric_mul gets slightly faster or slower.  Obviously a *big*
> performance drop might get pushback.

Thanks for guidance. Sounds reasonable to me.

Noted from 5e1f3b9eb:
"While it adds some space on 32-bit machines, we aren't optimizing for that 
case anymore."

In this case, the extra 32-bit numeric_mul code seems to be 89 lines of code, 
excluding comments.
To me, this seems like quite a lot, so I lean on thinking we should omit that 
code for now.
We can always add it later if we get pushback.

/Joel

Re: Logical Replication of sequences

[shveta malik]

On Mon, Aug 5, 2024 at 11:04 AM vignesh C  wrote:
>
> On Wed, 31 Jul 2024 at 14:39, shveta malik  wrote:
> >
> > On Mon, Jun 10, 2024 at 5:00 PM vignesh C  wrote:
> > >
> > > On Mon, 10 Jun 2024 at 12:24, Amul Sul  wrote:
> > > >
> > > >
> > > >
> > > > On Sat, Jun 8, 2024 at 6:43 PM vignesh C  wrote:
> > > >>
> > > >> On Wed, 5 Jun 2024 at 14:11, Amit Kapila  
> > > >> wrote:
> > > >> [...]
> > > >> A new catalog table, pg_subscription_seq, has been introduced for
> > > >> mapping subscriptions to sequences. Additionally, the sequence LSN
> > > >> (Log Sequence Number) is stored, facilitating determination of
> > > >> sequence changes occurring before or after the returned sequence
> > > >> state.
> > > >
> > > >
> > > > Can't it be done using pg_depend? It seems a bit excessive unless I'm 
> > > > missing
> > > > something.
> > >
> > > We'll require the lsn because the sequence LSN informs the user that
> > > it has been synchronized up to the LSN in pg_subscription_seq. Since
> > > we are not supporting incremental sync, the user will be able to
> > > identify if he should run refresh sequences or not by checking the lsn
> > > of the pg_subscription_seq and the lsn of the sequence(using
> > > pg_sequence_state added) in the publisher.
> >
> > How the user will know from seq's lsn that he needs to run refresh.
> > lsn indicates page_lsn and thus the sequence might advance on pub
> > without changing lsn and thus lsn may look the same on subscriber even
> > though a sequence-refresh is needed. Am I missing something here?
>
> When a sequence is synchronized to the subscriber, the page LSN of the
> sequence from the publisher is also retrieved and stored in
> pg_subscriber_rel as shown below:
> --- Publisher page lsn
> publisher=# select pg_sequence_state('seq1');
>  pg_sequence_state
> 
>  (0/1510E38,65,1,t)
> (1 row)
>
> --- Subscriber stores the publisher's page lsn for the sequence
> subscriber=# select * from pg_subscription_rel where srrelid = 16384;
>  srsubid | srrelid | srsubstate | srsublsn
> -+-++---
>16389 |   16384 | r  | 0/1510E38
> (1 row)
>
> If changes are made to the sequence, such as performing many nextvals,
> the page LSN will be updated. Currently the sequence values are
> prefetched for SEQ_LOG_VALS 32, so the lsn will not get updated for
> the prefetched values, once the prefetched values are consumed the lsn
> will get updated.
> For example:
> --- Updated LSN on the publisher (old lsn - 0/1510E38, new lsn - 0/1558CA8)
> publisher=# select pg_sequence_state('seq1');
>   pg_sequence_state
> --
>  (0/1558CA8,143,22,t)
> (1 row)
>
> The user can then compare this updated value with the sequence's LSN
> in pg_subscription_rel to determine when to re-synchronize the
> sequence.

Thanks for the details. But I was referring to the case where we are
in between pre-fetched values on publisher (say at 25th value), while
on subscriber we are slightly behind (say at 15th value), but page-lsn
will be the same on both. Since the subscriber is behind, a
sequence-refresh is needed on sub, but by looking at lsn (which is
same), one can not say that for sure.  Let me know if I have
misunderstood it.

thanks
Shveta

Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?

[Joe Conway]

On 7/15/24 17:42, Daniel Gustafsson wrote:

On 14 Jul 2024, at 14:03, Peter Eisentraut 
wrote:

On 12.07.24 21:42, Daniel Gustafsson wrote:

On 11 Jul 2024, at 23:22, Peter Eisentraut
 wrote: The 0001 patch removes the
functions pgtls_init_library() and pgtls_init() but keeps the
declarations in libpq-int.h.  This should be fixed.

Ah, nice catch. Done in the attached rebase.


This patch version looks good to me.


Thanks for review, I will go ahead with this once back from vacation
at the tail end of July when I can properly handle the BF.


Small comments on the commit message of 0002: Typo "checkig".
Also, maybe the commit message title can be qualified a little,
since we're not really doing "Remove pg_strong_random
initialization." but something like "Remove unnecessary ..."?


Good points, will address before pushing.


I know I am way late to this thread, and I have only tried a cursory 
skim of it given the length, but have we made any kind of announcement 
(packagers at least?) that we intend to not support Postgres 18 with ssl 
on RHEL 7.9 and derivatives? Yes, RHEL 7 just passed EOL, but there is 
commercial extended support available until July 2028[1] which means 
many people will continue to use it.


Joe

[1] 
https://www.redhat.com/en/blog/announcing-4-years-extended-life-cycle-support-els-red-hat-enterprise-linux-7

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Support specify tablespace for each merged/split partition

[Amul Sul]

On Mon, Jul 15, 2024 at 11:19 AM Junwang Zhao  wrote:
>
> In [1], it is suggested that it might be a good idea to support
> specifying the tablespace for each merged/split partition.
>
> We can do the following after this feature is supported:
>
> CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
> CREATE TABLE t (i int PRIMARY KEY) PARTITION BY RANGE (i);
> CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
> CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
>
> ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2 TABLESPACE tblspc;
>
> ALTER TABLE t SPLIT PARTITION tp_0_2 INTO
> (PARTITION tp_0_1 FOR VALUES FROM (0) TO (1) TABLESPACE tblspc,
> PARTITION tp_1_2 FOR VALUES FROM (1) TO (2));
>
> [1] 
> https://www.postgresql.org/message-id/abaf390b-3320-40a5-8815-ef476db5c...@oss.nttdata.com
>

+1 for this enhancement. Here are few comments for the patch:

-INTO partition_name
+   INTO partition_name [ TABLESPACE
tablespace_name ]

tablespace_name should be wrapped in the  tag, like partition_name.
--

 static Relation
-createPartitionTable(RangeVar *newPartName, Relation modelRel,
-AlterTableUtilityContext *context)
+createPartitionTable(RangeVar *newPartName, char *tablespacename,
+

The comment should mention the tablespace setting in the same way it
mentions the access method.
--

 /*
- * PartitionCmd - info for ALTER TABLE/INDEX ATTACH/DETACH PARTITION commands
+ * PartitionCmd - info for ALTER TABLE/INDEX
ATTACH/DETACH/MERGE/SPLIT PARTITION commands
  */

This change should be a separate patch since it makes sense
independently of your patch. Also, the comments for the "name"
variable in the same structure need to be updated.
--

+SELECT tablename, tablespace FROM pg_tables
+  WHERE tablename IN ('t', 'tp_0_2') AND schemaname = 'partitions_merge_schema'
+  ORDER BY tablename, tablespace;
+ tablename |tablespace
+---+--
+ t |
+ tp_0_2| regress_tblspace
+(2 rows)
+
+SELECT tablename, indexname, tablespace FROM pg_indexes
+  WHERE tablename IN ('t', 'tp_0_2') AND schemaname = 'partitions_merge_schema'
+  ORDER BY tablename, indexname, tablespace;
+ tablename |  indexname  | tablespace
+---+-+
+ t | t_pkey  |
+ tp_0_2| tp_0_2_pkey |
+(2 rows)
+

This seems problematic to me. The index should be in the same
tablespace as the table.
--

Please add the commitfest[1] entry if not already done.

1] https://commitfest.postgresql.org/

Regards,
Amul

BlastRADIUS mitigation

[Thomas Munro]

Hi,

(This discussion started on the security@ list, but was deemed
suitable for the -hackers@ list.)

PostgreSQL supports RADIUS authentication[1].  It's probably not
widely used, and generally any security technology based on MD5 alone
should by now be considered weak and obsolete, but there are a few
reports of users.  Recently, a paper was published[2] showing
successful attacks on a range of RADIUS clients, though no mention of
PostgreSQL.  It's not just an MD5 collision search, it's a more
specific cryptographic weakness in the protocol that allows messages
to be forged in quite practical amounts of CPU time.  We might be
quite lucky though: our hard-coded RADIUS_TIMEOUT = 3s doesn't seem to
be enough time, based on my non-crypto-expert reading of the paper at
least.

Even if we assume that is true, FreeRADIUS 3.2.5 has recently[3]
started spewing scary warnings when PostgreSQL sends requests to it by
default, and advising administrators to adjust the relevant setting
further to just drop unsigned messages.  I assume the commercial
RADIUS implementations will do something like that too, based on
comments about the intended direction and expectations of clients in
an in-development RFC[4].

The attached patch-set adds a basic TAP test for RADIUS
authentication, and then adds a Message-Authenticator attribute to all
outgoing requests (an HMAC-MD5 signature for the whole message that
was already defined by the RFCs but marked optional and often
omitted), and also *optionally* requires a Message-Authenticator to
appear in responses from the RADIUS server, and verifies it if
present.  With the change, FreeRADIUS 3.2.5 is happy to talk to
PostgreSQL again.

The response requirement can be enabled by radiusrequirema=1 in
pg_hba.conf.  For example, Debian stable is currently shipping
FreeRADIUS 3.2.1 which doesn't yet send the MA in its responses, but
FreeBSD and Debian "testing" have started shipping FreeRADIUS 3.2.5
which is how I noticed all this.  So it doesn't seem quite right to
require it by default, yet?

It's quite easy to test this locally, if you have FreeRADIUS installed
on any Unixoid system.  See the TAP test for some minimal
configuration files required to try it out.

I had originally developed the TAP test as part of a larger project[5]
really about something else, which is why it has Michael listed as a
reviewer already, and this version incorporates some new improvements
he recommended (thanks!).  I've created this new thread and new
minimal test just to deal with the BlastRADIUS mitigation topic.

We might also consider just dropping RADIUS support in 18, if we don't
get patches to bring it up to date with modern RADIUS recommendations
beyond the mitigation (deprecating UDP, adding TLS, probably more
things).  Such patches would ideally be written by someone with a more
direct interest in the protocol (ie I am not volunteering).  But even
if we decide to drop it instead.  I think we'd still want the change
I'm proposing here for released branches.

Since PostgreSQL v12 and v13 don't have the modern "common/hmac.h"
API, I came up with a cheap kludge: locally #define those interfaces
to point directly to the OpenSSL HMAC API, or just give up and drop
Message-Authenticator support if you didn't build with OpenSSL support
(in practice everyone does).  Better ideas?

[1] https://www.postgresql.org/docs/current/auth-radius.html
[2] https://www.blastradius.fail/
[3] 
https://github.com/FreeRADIUS/freeradius-server/commit/6616be90346beb6050446bd00c8ed5bca1b8ef29
[4] https://datatracker.ietf.org/doc/draft-ietf-radext-deprecating-radius/
[5] 
https://www.postgresql.org/message-id/flat/CA%2BhUKGKxNoVjkMCksnj6z3BwiS3y2v6LN6z7_CisLK%2Brv%2B0V4g%40mail.gmail.com


v1-0001-Add-simple-test-for-RADIUS-authentication.patch
Description: Binary data


v1-0002-ci-Enable-RADIUS-tests.patch
Description: Binary data


v1-0003-Mitigation-for-BlastRADIUS.patch
Description: Binary data


v1-0004-Teach-007_radius-test-about-Message-Authenticator.patch
Description: Binary data


v1-0005-XXX-BlastRADIUS-back-patch-kludge-for-12-and-13.patch
Description: Binary data

Re: Is *fast* 32-bit support still important?

[Joel Jacobson]

On Mon, Aug 5, 2024, at 14:24, Aleksander Alekseev wrote:
>> Could we add a text message that is displayed to a user,
>> when compiling PostgreSQL on a 32-bit platform?
>
> What would be actionable items depending on the results? Option A:
> someone claims to use PostgreSQL on 32-bit hardware. Option B: no one
> admits that they use PostgreSQL on 32-bit hardware (but maybe someone
> actually does and/or will in the future). Regardless of the results
> you can't drop the support of 32-bit software (until it gets as
> difficult and pointless as with AIX that was dropped recently) and it
> will not tell you how slow the 32-bit version of PostgreSQL can be.
>
> If there are no actionable items why create a poll?

Never suggested *dropping* 32-bit support; that's a different question.

However, if we were to receive input from 32-bit PostgreSQL users explaining
why they need *high performance*, then I imagine that could affect how we
feel about 32-bit optimizations.

Right now, there doesn't seem to be a consensus on whether we should optimize
for 32-bit or not.

On the one hand, we have commit 5e1f3b9 that says:
"While it adds some space on 32-bit machines, we aren't optimizing
for that case anymore."

On the other hand, we have the ongoing work on optimizing numeric_mul,
where a patch is suggested with extra code to optimize for 32-bit.

I agree, however, that the absence of any comments from such a poll would not
give us any more information.

/Joel

Re: SQL Property Graph Queries (SQL/PGQ)

[Ashutosh Bapat]

Hi Imran,

On Sun, Aug 4, 2024 at 12:32 PM Imran Zaheer  wrote:
>
> Hi
> I am attaching a new patch for a minor feature addition.
>
> - Adding support for 'Labels and properties: EXCEPT list'

Do you intend to support EXCEPT in the label expression as well or
just properties?

>
> Please let me know if something is missing.

I think the code changes are in the right place. I didn't review the
patch thoroughly. But here are some comments and some advice.

Please do not top-post on hackers.

Always sent the whole patchset. Otherwise, CI bot gets confused. It
doesn't pick up patchset from the previous emails.

About the functionality: It's not clear to me whether an EXCEPT should
be applicable only at the time of property graph creation or it should
be applicable always. I.e. when a property graph is dumped, should it
have EXCEPT in it or have a list of columns surviving except list?
What if a column in except list is dropped after creating a property
graph?

Some comments on the code
1. You could use list_member() in insert_property_records() to check
whether a given column is in the list of exceptions after you have
enveloped in String node.
2. The SELECT with GRAPH_TABLE queries are tested in graph_table.sql.
We don't include those in create_property_graph.sql
3. Instead of creating a new property graph in the test, you may
modify one of the existing property graphs to have a label with except
list and then query it.

We are aiming a minimal set of features in the first version. I will
let Peter E. decide whether to consider this as minimal set feature or
not. The feature looks useful to me.

-- 
Best Wishes,
Ashutosh Bapat

Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?

[Devrim Gündüz]

Hi Joe,

On Mon, 2024-08-05 at 08:38 -0400, Joe Conway wrote:
> I know I am way late to this thread, and I have only tried a cursory 
> skim of it given the length, but have we made any kind of announcement
> (packagers at least?) that we intend to not support Postgres 18 with
> ssl  on RHEL 7.9 and derivatives?

I dropped RHEL 7 support as of PostgreSQL 16:
https://yum.postgresql.org/news/rhel7-postgresql-rpms-end-of-life/

and also recently stopped providing updates except PostgreSQL packages:

https://yum.postgresql.org/news/rhel7-end-of-life/

Min OS version is RHEL 8 / SLES 15.

-HTH

Regards,
-- 
Devrim Gündüz
Open Source Solution Architect, PostgreSQL Major Contributor
Twitter: @DevrimGunduz , @DevrimGunduzTR


signature.asc
Description: This is a digitally signed message part

Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?

[Tom Lane]

Joe Conway  writes:
> I know I am way late to this thread, and I have only tried a cursory 
> skim of it given the length, but have we made any kind of announcement 
> (packagers at least?) that we intend to not support Postgres 18 with ssl 
> on RHEL 7.9 and derivatives? Yes, RHEL 7 just passed EOL, but there is 
> commercial extended support available until July 2028[1] which means 
> many people will continue to use it.

PG v16 will be in-support until November 2028, so it's not like
we are leaving RHEL 7 completely in the lurch.  I doubt that the
sort of people who are still running an EOL OS are looking to put
a bleeding-edge database on it, so this seems sufficient to me.

As for notifying packagers --- Red Hat themselves will certainly
not be trying to put new major versions of anything on RHEL 7,
and Devrim has stopped packaging newer PG for RHEL 7 altogether,
so who among them is going to care?

regards, tom lane

Re: [PATCH] Add crc32(text) & crc32(bytea)

[Aleksander Alekseev]

Hi,

> I'm curious why we need to do this instead of only using the macros:
>
> INIT_TRADITIONAL_CRC32(crc);
> COMP_TRADITIONAL_CRC32(crc, VARDATA_ANY(in), len);
> FIN_TRADITIONAL_CRC32(crc);
>
> + * IDENTIFICATION
> + *src/backend/utils/adt/hashfuncs.c
>
> Perhaps these would fit into src/backend/utils/hash/pg_crc.c?

Thanks, PFA patch v3.

--
Best regards,
Aleksander Alekseev


v3-0001-Add-crc32-bytea-crc32c-bytea.patch
Description: Binary data

Re: Detailed release notes

[Daniel Gustafsson]

> On 5 Aug 2024, at 13:16, Marcos Pegoraro  wrote:
> 
> Em seg., 5 de ago. de 2024 às 07:54, jian he  > escreveu:
>> 
>> [commitId_link1, commitId_link2]: Change functions to use a safe
>> search_path during maintenance operations (Jeff Davis)
> 
> I don't like that prefix dirtying each item. 

I too would prefer links at the end, not least since we might have 2 or 3 (or
more) links for an item.

Python also links to the Github issue and not the commit, in our project the
analog to that would be linking to the thread in the mailinglist archive as
well as the commit.  For us linking to the commit is probably preferrable since
it will link to the thread but the thread often wont link to the commit (like a
Github issue will).  Maybe icons for code/emailthread can be used to make it
clear what the link is, and cut down to horizontal space required?

--
Daniel Gustafsson

Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?

[Joe Conway]

On 8/5/24 09:14, Tom Lane wrote:

Joe Conway  writes:
I know I am way late to this thread, and I have only tried a cursory 
skim of it given the length, but have we made any kind of announcement 
(packagers at least?) that we intend to not support Postgres 18 with ssl 
on RHEL 7.9 and derivatives? Yes, RHEL 7 just passed EOL, but there is 
commercial extended support available until July 2028[1] which means 
many people will continue to use it.


PG v16 will be in-support until November 2028, so it's not like
we are leaving RHEL 7 completely in the lurch.  I doubt that the
sort of people who are still running an EOL OS are looking to put
a bleeding-edge database on it, so this seems sufficient to me.


ok


As for notifying packagers --- Red Hat themselves will certainly
not be trying to put new major versions of anything on RHEL 7,
and Devrim has stopped packaging newer PG for RHEL 7 altogether,
so who among them is going to care?



Perhaps no one on packagers. It would not shock me to see complaints 
from others after we rip out support for 1.0.2, but maybe not ¯\_(ツ)_/¯


--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Remove obsolete RECHECK keyword completely

[Aleksander Alekseev]

Hi,

> I propose to remove the obsolete RECHECK keyword completely.  This used
> to be part of CREATE OPERATOR CLASS and ALTER OPERATOR FAMILY, but it
> has done nothing (except issue a NOTICE) since PostgreSQL 8.4.  Commit
> 30e7c175b81 removed support for dumping from pre-9.2 servers, so this no
> longer serves any need, it seems to me.
>
> This now removes it completely, and you'd get a normal parse error if
> you used it.

I reviewed and tested the code. LGTM.

--
Best regards,
Aleksander Alekseev

Re: meson "experimental"?

[Aleksander Alekseev]

Hi,

> Agreed.  I've pushed your patch for that:
>
> > Here are corresponding patches.
>
> The  ID is new in v17, so I also renamed it like
> s/installation-notes-visual/installation-notes-visual-studio/.
>
> (I didn't examine or push your other patch, which was about $SUBJECT.)

Thanks. Re-attaching 0001 and adding it to the nearest CF to make it
visible on cfbot.

-- 
Best regards,
Aleksander Alekseev


v1-0001-Meson-is-not-experimental-on-Windows.patch
Description: Binary data

Re: Remove support for old realpath() API

[Tom Lane]

Heikki Linnakangas  writes:
> We don't seem to have any mentions of POSIX or SuS in docs, in the 
> installation sections. There are a few mentions of POSIX-1.2008 and 
> POSIX-1.2001 it in the commit log, though, where we require features 
> specified by those. Can we rely on everything from POSIX-1-2008 
> nowadays, or is it more on a case-by-case basis, depending on which 
> parts of POSIX are supported by various platforms?

I'd say it's still case-by-case.  Perhaps everything in POSIX-1.2008
is supported now on every platform we care about, but perhaps not.

regards, tom lane

Re: BlastRADIUS mitigation

[Heikki Linnakangas]

On 05/08/2024 15:43, Thomas Munro wrote:

The response requirement can be enabled by radiusrequirema=1 in
pg_hba.conf.  For example, Debian stable is currently shipping
FreeRADIUS 3.2.1 which doesn't yet send the MA in its responses, but
FreeBSD and Debian "testing" have started shipping FreeRADIUS 3.2.5
which is how I noticed all this.  So it doesn't seem quite right to
require it by default, yet?


Agreed.


Since PostgreSQL v12 and v13 don't have the modern "common/hmac.h"
API, I came up with a cheap kludge: locally #define those interfaces
to point directly to the OpenSSL HMAC API, or just give up and drop
Message-Authenticator support if you didn't build with OpenSSL support
(in practice everyone does).  Better ideas?


Seems reasonable. It probably wouldn't be hard to backport common/hmac.h 
either, perhaps in a limited fashion with just md5 support.


Review on v1-0001-Add-simple-test-for-RADIUS-authentication.patch:


+if ($ENV{PG_TEST_EXTRA} !~ /\bradius\b/)
+{
+   plan skip_all => 'radius not enabled in PG_TEST_EXTRA';
+}
+elsif ($^O eq 'freebsd')
+{
+   $radiusd = '/usr/local/sbin/radiusd';
+}
+elsif ($^O eq 'linux' && -f '/usr/sbin/freeradius')
+{
+   $radiusd = '/usr/sbin/freeradius';
+}
+elsif ($^O eq 'linux')
+{
+   $radiusd = '/usr/sbin/radiusd';
+}
+elsif ($^O eq 'darwin' && -d '/opt/local')
+{
+   # typical path for MacPorts
+   $radiusd = '/opt/local/sbin/radiusd';
+   $radiusd_prefix = '/opt/local';
+}
+elsif ($^O eq 'darwin' && -d '/opt/homebrew')
+{
+   # typical path for Homebrew on ARM
+   $radiusd = '/opt/homebrew/bin/radiusd';
+   $radiusd_prefix = '/opt/homebrew';
+}
+elsif ($^O eq 'darwin' && -d '/usr/local')
+{
+   # typical path for Homebrew on Intel
+   $radiusd = '/usr/local/bin/radiusd';
+   $radiusd_prefix = '/usr/local';
+}
+else
+{
+   plan skip_all =>
+ "radius tests not supported on $^O or dependencies not installed";
+}


Seems that on linux or freebsd, you'd plow ahead even if the binary is 
not found, and fail later, while on macOS you'd skip the tests. I think 
we should always error out if the dependencies are not found. If you 
make an effort to add PG_TEST_EXTRA=radius, presumably you really do 
want to run the tests, and if dependencies are missing you'd like to 
know about it.



+  secret = "shared-secret"


Let's use a random value for this, and for the Cleartext-Password. This 
only runs locally, and only if you explicitly add it to PG_TEST_EXTRA, 
but it still seems nice to protect from other users on the system when 
we can do so easily.



+security {
+  require_message_authenticator = "yes"
+}



+# Note that require_message_authenticator defaulted to "no" before 3.2.5, and
+# then switched to "auto" (a new mode that fills the logs up with warning
+# messages about clients that don't send MA), and presumably a later version
+# will default to "yes".


That's not quite accurate: the option didn't exist before version 3.2.5. 
What happens if you set it on an older server version? /me tests: seems 
that FreeRadius 3.2.1 silently accepts the setting, or any other setting 
it doesn't recognize, and will do nothing with it. A little surprising, 
but ok. I didn't find any mention in the docs on that.


(Also, that will make the test fail, until the 
v1-0003-Mitigation-for-BlastRADIUS.patch is also applied. You could 
leave that out of the test in this first patch, and add it 
v1-0003-Mitigation-for-BlastRADIUS.patch)


Review on v1-0003-Mitigation-for-BlastRADIUS.patch:


+  
+   radiusrequirema
+   
+
+ Whether to require a valid Message-Authenticator
+ attribute in messages received from RADIUS servers, and ignore 
messages
+ that don't contain it.  The default value
+ is 0, but it can be set to 1
+ to enable that requirement.
+ This setting does not affect requests sent by
+ PostgreSQL to the RADIUS server, which
+ always include a Message-Authenticator attribute
+ (but didn't in earlier releases).
+
+   
+  


I think this should include some advise on why and when you should set 
it. Something like:


Enabling this mitigates the RADIUS protocol vulnerability described at 
blastradius.fail. It is recommended to always set this to 1, unless you 
are running an older RADIUS server version that does include the 
mitigation to include Message-Authenticator in all replies. The default 
will be changed to 1 in a future PostgreSQL version.



attr = (radius_attribute *) (packet + RADIUS_HEADER_LENGTH);

while ((uint8 *) attr < end)
{
/* Would this attribute overflow the buffer? */
if (&attr->length >= end || (uint8 *) attr + attr->length > end)
return false;


Is this kind of pointer arithmetic safe? In theory, if a packet is 
malformed so that attr->length points beyond the end of the packet, 
"(uint8 *) attr + at

Re: Amcheck verification of GiST and GIN

[Tomas Vondra]

Hi,

I've spent a bit more time looking at the GiST part as part of my
"parallel GiST build" patch nearby, and I think there's some sort of
memory leak.

Consider this:

  create table t (a text);

  insert into t select md5(i::text)
from generate_series(1,2500) s(i);

  create index on t using gist (a gist_trgm_ops);

  select gist_index_check('t_a_idx', true);

This creates a ~4GB GiST trigram index, and then checks it. But that
gets killed, because of OOM killer. On my test machine it consumes
~6.5GB of memory before OOM intervenes.

The memory context stats look like this:

  TopPortalContext: 8192 total in 1 blocks; 7680 free (0 chunks); 512 used
PortalContext: 1024 total in 1 blocks; 616 free (0 chunks); 408
used: 
  ExecutorState: 8192 total in 1 blocks; 4024 free (4 chunks); 4168 used
printtup: 8192 total in 1 blocks; 7952 free (0 chunks); 240 used
ExprContext: 8192 total in 1 blocks; 7224 free (10 chunks); 968 used
  amcheck context: 3128950872 total in 376 blocks; 219392 free
(1044 chunks); 3128731480 used
ExecutorState: 8192 total in 1 blocks; 7200 free (0 chunks);
992 used
  ExprContext: 8192 total in 1 blocks; 7952 free (0 chunks);
240 used
GiST scan context: 22248 total in 2 blocks; 7808 free (8
chunks); 14440 used

This is from before the OOM kill, but it shows there's ~3GB of memory is
the amcheck context.

Seems like a memory leak to me - I didn't look at which place leaks.



regards

-- 
Tomas Vondra

Re: psql client does not handle WSAEWOULDBLOCK on Windows

[Umar Hayat]

I have not reproduce your test scenario, looking at code please see following 
comments:

If you check the function definition of pqsecure_raw_read() it actually do set 
errno like bellow

SOCK_ERRNO_SET(result_errno);
where result_errno = SOCK_ERRNO

Means anybody using those function pqsecure_raw_read/write, does not need to 
take care of portable ERRNO.

Regards
Umar Hayat

The new status of this patch is: Waiting on Author

Re: WIP: parallel GiST index builds

[Tomas Vondra]

Hi,

Here's an updated patch using GetFakeLSNForUnloggedRel() instead of the
atomic counter. I think this looks much nicer and less invasive, as it
simply uses XLogCtl shared memory (instead of having to pass a new
pointer everywhere).

We still need to pass the is_parallel flag, though. I wonder if we could
get rid of that too, and just use GetFakeLSNForUnloggedRel() for both
parallel and non-parallel builds? Why wouldn't that work?

I've spent quite a bit of time testing this, but mostly for correctness.
I haven't redone the benchmarks, that's on my TODO.


regards

-- 
Tomas VondraFrom c2766218c657097bb969c97d664c12803b374ff0 Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Sun, 26 May 2024 21:44:27 +0200
Subject: [PATCH v20240805 1/6] WIP parallel GiST build

Implements parallel GiST index build for the unsorted case. The build
simply starts parallel workers that insert values into the index the
usual way (as if there were multiple clients doing INSERT).

The basic infrastructure is copied from parallel BRIN builds (and also
from the nearby parallel GIN build). There's nothing particularly
special or interesting, except for the gistBuildParallelCallback()
callback. The two significant changes in the callback are:

1) disabling buffering

Buffered builds assume the worker is the only backend that can split
index pages etc. With serial workers that is trivially true, but with
parallel workers this leads to confusion.

In principle this is solvable by moving the buffers into shared memory
and coordinating the workers (locking etc.). But the patch does not do
that yet - it's clearly non-trivial, and I'm not really convinced it's
worth it.

2) generating "proper" fake LSNs

The serial builds disable all WAL-logging for the index, until the very
end when the whole index is WAL-logged. This however also means we don't
set page LSNs on the index pages - but page LSNs are used to detect
concurrent changes to the index structure (e.g. page splits). For serial
builds this does not matter, because only the build worker can modify
the index, so it just sets the same LSN "1" for all pages. Both of this
(disabling WAL-logging and using bogus page LSNs) is controlled by the
same flag is_build.

Having the same page LSN does not work for parallel builds, as it would
mean workers won't notice splits done by other workers, etc.

One option would be to set is_bild=false, which enables WAL-logging, as
if during regular inserts, and also assigns proper page LSNs. But we
don't want to WAL-log everything, that's unnecessary. We want to only
start WAL-logging the index once the build completes, just like for
serial builds. And only do the fake LSNs, as for unlogged indexes etc.

So this introduces a separate flag is_parallel, which forces generating
the "proper" fake LSN. But we can still do is_build=true, and only log
the index at the end of the build.
---
 src/backend/access/gist/gist.c|  37 +-
 src/backend/access/gist/gistbuild.c   | 713 +-
 src/backend/access/gist/gistutil.c|  10 +-
 src/backend/access/gist/gistvacuum.c  |   6 +-
 src/backend/access/transam/parallel.c |   4 +
 src/include/access/gist_private.h |  12 +-
 src/tools/pgindent/typedefs.list  |   2 +
 7 files changed, 739 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index ed4ffa63a77..f5f56fb2503 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -75,7 +75,7 @@ gisthandler(PG_FUNCTION_ARGS)
 	amroutine->amclusterable = true;
 	amroutine->ampredlocks = true;
 	amroutine->amcanparallel = false;
-	amroutine->amcanbuildparallel = false;
+	amroutine->amcanbuildparallel = true;
 	amroutine->amcaninclude = true;
 	amroutine->amusemaintenanceworkmem = false;
 	amroutine->amsummarizing = false;
@@ -182,7 +182,7 @@ gistinsert(Relation r, Datum *values, bool *isnull,
 		 values, isnull, true /* size is currently bogus */ );
 	itup->t_tid = *ht_ctid;
 
-	gistdoinsert(r, itup, 0, giststate, heapRel, false);
+	gistdoinsert(r, itup, 0, giststate, heapRel, false, false);
 
 	/* cleanup */
 	MemoryContextSwitchTo(oldCxt);
@@ -230,7 +230,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 List **splitinfo,
 bool markfollowright,
 Relation heapRel,
-bool is_build)
+bool is_build,
+bool is_parallel)
 {
 	BlockNumber blkno = BufferGetBlockNumber(buffer);
 	Page		page = BufferGetPage(buffer);
@@ -501,9 +502,17 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 		 * smaller than any real or fake unlogged LSN that might be generated
 		 * later. (There can't be any concurrent scans during index build, so
 		 * we don't need to be able to detect concurrent splits yet.)
+		 *
+		 * However, with a parallel index build, we need to assign valid LSN,
+		 * as it's used to detect concurrent index modifications.
 		 */
 		if (is_build)
-			recptr = GistBuildLSN;
+		{
+			if (is_para

Re: [PATCH] Add crc32(text) & crc32(bytea)

[Nathan Bossart]

On Mon, Aug 05, 2024 at 04:19:45PM +0300, Aleksander Alekseev wrote:
> Thanks, PFA patch v3.

This looks pretty good to me.  The only point that I think deserves more
discussion is the return type.  Does bytea make the most sense here?  Or
should we consider int/bigint?

-- 
nathan

Re: Support specify tablespace for each merged/split partition

[Junwang Zhao]

Hi Amul,

Thanks for your review.

On Mon, Aug 5, 2024 at 8:38 PM Amul Sul  wrote:
>
> On Mon, Jul 15, 2024 at 11:19 AM Junwang Zhao  wrote:
> >
> > In [1], it is suggested that it might be a good idea to support
> > specifying the tablespace for each merged/split partition.
> >
> > We can do the following after this feature is supported:
> >
> > CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
> > CREATE TABLE t (i int PRIMARY KEY) PARTITION BY RANGE (i);
> > CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
> > CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
> >
> > ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2 TABLESPACE 
> > tblspc;
> >
> > ALTER TABLE t SPLIT PARTITION tp_0_2 INTO
> > (PARTITION tp_0_1 FOR VALUES FROM (0) TO (1) TABLESPACE tblspc,
> > PARTITION tp_1_2 FOR VALUES FROM (1) TO (2));
> >
> > [1] 
> > https://www.postgresql.org/message-id/abaf390b-3320-40a5-8815-ef476db5c...@oss.nttdata.com
> >
>
> +1 for this enhancement. Here are few comments for the patch:
>
> -INTO partition_name
> +   INTO  class="parameter">partition_name [ TABLESPACE
> tablespace_name ]
>
> tablespace_name should be wrapped in the  tag, like 
> partition_name.

Will add in next version.

> --
>
>  static Relation
> -createPartitionTable(RangeVar *newPartName, Relation modelRel,
> -AlterTableUtilityContext *context)
> +createPartitionTable(RangeVar *newPartName, char *tablespacename,
> +
>
> The comment should mention the tablespace setting in the same way it
> mentions the access method.

I'm not good at wording, can you give some advice?

> --
>
>  /*
> - * PartitionCmd - info for ALTER TABLE/INDEX ATTACH/DETACH PARTITION commands
> + * PartitionCmd - info for ALTER TABLE/INDEX
> ATTACH/DETACH/MERGE/SPLIT PARTITION commands
>   */
>
> This change should be a separate patch since it makes sense
> independently of your patch. Also, the comments for the "name"
> variable in the same structure need to be updated.

Will be split into a separate patch in the next version.

> --
>
> +SELECT tablename, tablespace FROM pg_tables
> +  WHERE tablename IN ('t', 'tp_0_2') AND schemaname = 
> 'partitions_merge_schema'
> +  ORDER BY tablename, tablespace;
> + tablename |tablespace
> +---+--
> + t |
> + tp_0_2| regress_tblspace
> +(2 rows)
> +
> +SELECT tablename, indexname, tablespace FROM pg_indexes
> +  WHERE tablename IN ('t', 'tp_0_2') AND schemaname = 
> 'partitions_merge_schema'
> +  ORDER BY tablename, indexname, tablespace;
> + tablename |  indexname  | tablespace
> +---+-+
> + t | t_pkey  |
> + tp_0_2| tp_0_2_pkey |
> +(2 rows)
> +
>
> This seems problematic to me. The index should be in the same
> tablespace as the table.

I'm not sure about this, it seems to me that partition index will alway
inherit the tablespaceId of its parent(see generateClonedIndexStmt),
do you think we should change the signature of this function?

One thing worth mentioning is that for UNIQUE/PRIMARY KEY,
it allows setting *USING INDEX TABLESPACE tablespace_name*,
I don't think we should change the index tablespace in this case,
what do you think?

> --
>
> Please add the commitfest[1] entry if not already done.
>
> 1] https://commitfest.postgresql.org/

https://commitfest.postgresql.org/49/5157/

I have added you as a reviewer, hope you don't mind.

>
> Regards,
> Amul



-- 
Regards
Junwang Zhao

Re: Detailed release notes

[Euler Taveira]

On Mon, Aug 5, 2024, at 10:33 AM, Daniel Gustafsson wrote:
>> On 5 Aug 2024, at 13:16, Marcos Pegoraro  wrote:
>> 
>> Em seg., 5 de ago. de 2024 às 07:54, jian he  
>> escreveu:
>>> 
>>> [commitId_link1, commitId_link2]: Change functions to use a safe
>>> search_path during maintenance operations (Jeff Davis)
>> 
>> I don't like that prefix dirtying each item. 
> 
> I too would prefer links at the end, not least since we might have 2 or 3 (or
> more) links for an item.

+1.

> Python also links to the Github issue and not the commit, in our project the
> analog to that would be linking to the thread in the mailinglist archive as
> well as the commit.  For us linking to the commit is probably preferrable 
> since
> it will link to the thread but the thread often wont link to the commit (like 
> a
> Github issue will).  Maybe icons for code/emailthread can be used to make it
> clear what the link is, and cut down to horizontal space required?

PgBouncer adds the PR number at the end [1] too. I generally prefer this style
because you read the message and after that if you want additional detail you
can click on the link at the end.

I don't know if linking to the thread is a good idea. We have long long threads
that cannot provide quick information for the reader. Since we don't have a
concept of every commit has a CF entry, IMO we should use only commits here. The
commit message points to the discussion so it is a good start point if you want
to research about that specific feature.

If one commit is not sufficient for that item, we can always add multiple
commits as we usually do in the release notes comments.


[1] https://www.pgbouncer.org/changelog.html


--
Euler Taveira
EDB   https://www.enterprisedb.com/

Re: pg_verifybackup: TAR format backup verification

[Robert Haas]

On Fri, Aug 2, 2024 at 7:43 AM Amul Sul  wrote:
> Please consider the attached version for the review.

Thanks. I committed 0001-0003. The only thing that I changed was that
in 0001, you forgot to pgindent, which actually mattered quite a bit,
because astreamer is one character shorter than bbstreamer.

Before we proceed with the rest of this patch series, I think we
should fix up the comments for some of the astreamer files. Proposed
patch for that attached; please review.

I also noticed that cfbot was unhappy about this patch set:

[10:37:55.075] pg_verifybackup.c:100:7: error: no previous extern
declaration for non-static variable 'format'
[-Werror,-Wmissing-variable-declarations]
[10:37:55.075] charformat = '\0';  /* p(lain)/t(ar) */
[10:37:55.075] ^
[10:37:55.075] pg_verifybackup.c:100:1: note: declare 'static' if the
variable is not intended to be used outside of this translation unit
[10:37:55.075] charformat = '\0';  /* p(lain)/t(ar) */
[10:37:55.075] ^
[10:37:55.075] pg_verifybackup.c:101:23: error: no previous extern
declaration for non-static variable 'compress_algorithm'
[-Werror,-Wmissing-variable-declarations]
[10:37:55.075] pg_compress_algorithm compress_algorithm = PG_COMPRESSION_NONE;
[10:37:55.075]   ^
[10:37:55.075] pg_verifybackup.c:101:1: note: declare 'static' if the
variable is not intended to be used outside of this translation unit
[10:37:55.075] pg_compress_algorithm compress_algorithm = PG_COMPRESSION_NONE;
[10:37:55.075] ^
[10:37:55.075] 2 errors generated.

Please fix and, after posting future versions of the patch set, try to
remember to check http://cfbot.cputube.org/amul-sul.html

-- 
Robert Haas
EDB: http://www.enterprisedb.com


v6-0001-Improve-file-header-comments-for-astramer-code.patch
Description: Binary data

Re: Is *fast* 32-bit support still important?

[Robert Haas]

On Mon, Jul 29, 2024 at 4:41 PM Joel Jacobson  wrote:
> To me, it's non-obvious whether introducing `#if SIZEOF_DATUM < 8` with
> separate 32-bit and 64-bit code paths is worthwhile to maintain performance
> for both.

I feel like it's probably mostly up to the patch author. Reviewers
and/or the eventual committer might propose to reverse whatever
decision the patch author made, but personally, I wouldn't bounce a
patch for either having or not having separate code paths, unless
there was some particular reason to believe that whatever decision the
patch author made was likely to cause a problem.

One generally difficult thing about working in the PostgreSQL
community is that we have pretty limited formal decision-making
structures. A consensus on the list is valid only until the issue is
relitigated, which can happen at any time and for any reason. With
infrequent exceptions such as RMT or core pronouncements, a previous
consensus doesn't bind people who weren't participants in the previous
discussion, or even people who were. Everybody is free to change their
mind at any time. For that reason, I often find it more helpful to
approach questions like this from a pragmatic standpoint: rather than
asking "what is the policy about X?" I find it better to ask "If I do
X, what is probably going to happen?"

What I think is: if you write a patch that caters mostly to 64-bit
systems, probably nobody will notice or care that the 32-bit
performance is not great, because there just aren't that many 32-bit
users left out there. I think it's been a very long time since we got
a complaint about 32-bit performance, or a patch to improve 32-bit
performance, but we get other kinds of performance-optimizing patches
all the time, for all sorts of things. Perhaps that's because we
haven't yet done anything too terrible to 32-bit performance, but it's
probably also that if you're running a 32-bit system in 2024, you're
probably not expecting it to come under serious load. You likely care
more about getting the job done with limited machine resources than
anything else. And you likely expect it to be slow. I don't actually
know how much we'd have to regress 32-bit performance before users
started to show up and complain about it, and I'm certainly not
recommending that we do so gratuitously. At the same time, evidence
that people are displeased with our current 32-bit performance or that
they care about improving our 32-bit performance is very thin on the
ground, AFAIK. If at some point people do start showing up to
complain, then we'll know we've gone too far and we can back off --
with the benefit of knowing what people are actually unhappy about at
that point, vs. what we think they might be unhappy about.

And on the other hand, if you take the opposite approach and write a
patch that includes a separate code path for 32-bit systems, as long
as it works and doesn't look terribly ugly, I bet nobody's going to
waste time demanding that you rip it out.

So I would just do whatever you feel like doing and then defend your
choice in the comments and/or commit message.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: BlastRADIUS mitigation

[Tom Lane]

Heikki Linnakangas  writes:
> On 05/08/2024 15:43, Thomas Munro wrote:
>> The response requirement can be enabled by radiusrequirema=1 in
>> pg_hba.conf.  For example, Debian stable is currently shipping
>> FreeRADIUS 3.2.1 which doesn't yet send the MA in its responses, but
>> FreeBSD and Debian "testing" have started shipping FreeRADIUS 3.2.5
>> which is how I noticed all this.  So it doesn't seem quite right to
>> require it by default, yet?

> Agreed.

We should think about that not in terms of the situation today,
but the situation when we ship this fix, possibly as much as
three months from now.  (There was some mention in the security-list
discussion of maybe making an off-cycle release to get this out
sooner; but nothing was decided, and I doubt we'll do that unless
we start getting user complaints.)  It seems likely to me that
most up-to-date systems will have BlastRADIUS mitigation in place
by then, so maybe we should lean towards secure-by-default.

We don't necessarily have to make that decision today, either.
We could start with not-secure-by-default but reconsider
whenever the release is imminent.

regards, tom lane

Re: POC, WIP: OR-clause support for indexes

[Alexander Korotkov]

On Mon, Jul 29, 2024 at 5:36 AM Alexander Korotkov  wrote:
> On Sun, Jul 28, 2024 at 12:59 PM Alena Rybakina
> > Because of these reasons, I tried to save this and that transformation
> > together for each column and try to analyze for each expr separately
> > which method would be optimal.
>
> Yes, with v27 of the patch, optimization wouldn't work in these cases.
> However, you are using quite small table.  If you will use larger
> table or disable sequential scans, there would be bitmap plans to
> handle these queries.  So, v27 doesn't make the situation worse. It
> just doesn't optimize all that it could potentially optimize and
> that's OK.
>
> I've written a separate 0002 patch to address this.  Now, before
> generation of paths for bitmap OR, similar OR entries are grouped
> together.  When considering a group of similar entries, they are
> considered both together and one-by-one.  Ideally we could consider
> more sophisticated grouping, but that seems fine for now.  You can
> check how this patch handles the cases of above.
>
> Also, 0002 address issue of duplicated bitmap scan conditions in
> different forms. During generate_bitmap_or_paths() we need to exclude
> considered condition for other clauses.  It couldn't be as normal
> filtered out in the latter stage, because could reach the index in
> another form.
>
> > I agree with you that there is an overhead and your patch fixes this
> > problem, but optimizer needs to have a good ordering of expressions for
> > application.
> >
> > I think we can try to move the transformation to another place where
> > there is already a loop pass, and also save two options "OR" expr and
> > "ANY" expr in one place (through BoolExpr) (like find_duplicate_ors
> > function) and teach the optimizer to determine which option is better,
> > for example, like now in match_orclause_to_indexcol() function.
> >
> > What do you thing about it?
>
> find_duplicate_ors() and similar places were already tried before.
> Please, check upthread.  This approach receives severe critics. AFAIU,
> the problem is that find_duplicate_ors() during preprocessing, a
> cost-blind stage.
>
> This is why I'd like to continue developing ideas of v27, because it
> fits the existing framework.

The revised patchset is attached.  There is no material changes in the
logic, I found no issues here yet.  But it comes with refactoring,
cleanup, more comments and better commit messages.  I think now this
patchset is understandable and ready for review.

--
Regards,
Alexander Korotkov
Supabase


v30-0002-Teach-bitmap-path-generation-about-transforming-.patch
Description: Binary data


v30-0001-Transform-OR-clauses-to-SAOP-s-during-index-matc.patch
Description: Binary data

Re: Restart pg_usleep when interrupted

[Sami Imseih]

v4-0001-vaccum_delay-with-absolute-time-nanosleep.patch
Description: Binary data

> yeah, we already have a few macros that access the .ticks, so maybe we could 
> add
> 2 new ones, say:
> 
> 1. INSTR_TIME_ADD_MS(t1, msec)
> 2. INSTR_TIME_IS_GREATER(t1, t2)
> 
> I think the less operations is done in the while loop the better.
> 

See v4. it includes 2 new instr_time.h macros to simplify the 
code insidethe while loop.

Regards,

Sami

Re: POC, WIP: OR-clause support for indexes

[Alena Rybakina]

Ok, thank you for your work)

I think we can leave only the two added libraries in the first patch, 
others are superfluous.


On 05.08.2024 22:48, Alexander Korotkov wrote:

On Mon, Jul 29, 2024 at 5:36 AM Alexander Korotkov  wrote:

On Sun, Jul 28, 2024 at 12:59 PM Alena Rybakina

Because of these reasons, I tried to save this and that transformation
together for each column and try to analyze for each expr separately
which method would be optimal.

Yes, with v27 of the patch, optimization wouldn't work in these cases.
However, you are using quite small table.  If you will use larger
table or disable sequential scans, there would be bitmap plans to
handle these queries.  So, v27 doesn't make the situation worse. It
just doesn't optimize all that it could potentially optimize and
that's OK.

I've written a separate 0002 patch to address this.  Now, before
generation of paths for bitmap OR, similar OR entries are grouped
together.  When considering a group of similar entries, they are
considered both together and one-by-one.  Ideally we could consider
more sophisticated grouping, but that seems fine for now.  You can
check how this patch handles the cases of above.

Also, 0002 address issue of duplicated bitmap scan conditions in
different forms. During generate_bitmap_or_paths() we need to exclude
considered condition for other clauses.  It couldn't be as normal
filtered out in the latter stage, because could reach the index in
another form.


I agree with you that there is an overhead and your patch fixes this
problem, but optimizer needs to have a good ordering of expressions for
application.

I think we can try to move the transformation to another place where
there is already a loop pass, and also save two options "OR" expr and
"ANY" expr in one place (through BoolExpr) (like find_duplicate_ors
function) and teach the optimizer to determine which option is better,
for example, like now in match_orclause_to_indexcol() function.

What do you thing about it?

find_duplicate_ors() and similar places were already tried before.
Please, check upthread.  This approach receives severe critics. AFAIU,
the problem is that find_duplicate_ors() during preprocessing, a
cost-blind stage.

This is why I'd like to continue developing ideas of v27, because it
fits the existing framework.

The revised patchset is attached.  There is no material changes in the
logic, I found no issues here yet.  But it comes with refactoring,
cleanup, more comments and better commit messages.  I think now this
patchset is understandable and ready for review.

--
Regards,
Alexander Korotkov
Supabase


--
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
From 84e4c71dccd77810a770038be7d9104004726e1c Mon Sep 17 00:00:00 2001
From: Alena Rybakina 
Date: Mon, 5 Aug 2024 23:21:16 +0300
Subject: [PATCH 1/2] Transform OR-clauses to SAOP's during index matching

Replace "(indexkey op C1) OR (indexkey op C2) ... (indexkey op CN)" with
"indexkey op ANY(ARRAY[C1, C2, ...])" (ScalarArrayOpExpr node) during matching
a clause to index.

Here Ci is a i-th constant or parameters expression, 'expr' is non-constant
expression, 'op' is an operator which returns boolean result and has a commuter
(for the case of reverse order of constant and non-constant parts of the
expression, like 'Cn op expr').

This transformation allows to handle long OR-clauses with single IndexScan
avoiding slower bitmap scans.

Discussion: https://postgr.es/m/567ED6CA.2040504%40sigaev.ru
Author: Alena Rybakina 
Author: Andrey Lepikhov 
Reviewed-by: Peter Geoghegan 
Reviewed-by: Ranier Vilela 
Reviewed-by: Alexander Korotkov 
Reviewed-by: Robert Haas 
Reviewed-by: Jian He 
Reviewed-by: Tom Lane 
Reviewed-by: Nikolay Shaplov 
---
 src/backend/optimizer/path/indxpath.c  | 253 +
 src/test/regress/expected/create_index.out | 183 +--
 src/test/regress/expected/join.out |  57 -
 src/test/regress/sql/create_index.sql  |  42 
 src/test/regress/sql/join.sql  |   9 +
 5 files changed, 522 insertions(+), 22 deletions(-)

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index c0fcc7d78df..cb87126e758 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -32,7 +32,9 @@
 #include "optimizer/paths.h"
 #include "optimizer/prep.h"
 #include "optimizer/restrictinfo.h"
+#include "utils/array.h"
 #include "utils/lsyscache.h"
+#include "utils/syscache.h"
 #include "utils/selfuncs.h"
 
 
@@ -177,6 +179,10 @@ static IndexClause *match_rowcompare_to_indexcol(PlannerInfo *root,
  RestrictInfo *rinfo,
  int indexcol,
  IndexOptInfo *index);
+static IndexClause *match_orclause_to_indexcol(PlannerInfo *root,
+			   RestrictInfo *rinfo,
+			   int indexcol,
+			   IndexOptInfo *index);
 static IndexClause *expand_indexqual_rowcompare(PlannerInfo

Re: BlastRADIUS mitigation

[Thomas Munro]

On Tue, Aug 6, 2024 at 2:41 AM Heikki Linnakangas  wrote:
> Seems that on linux or freebsd, you'd plow ahead even if the binary is
> not found, and fail later, while on macOS you'd skip the tests. I think
> we should always error out if the dependencies are not found. If you
> make an effort to add PG_TEST_EXTRA=radius, presumably you really do
> want to run the tests, and if dependencies are missing you'd like to
> know about it.

Fixed.

> > +  secret = "shared-secret"
>
> Let's use a random value for this, and for the Cleartext-Password. This
> only runs locally, and only if you explicitly add it to PG_TEST_EXTRA,
> but it still seems nice to protect from other users on the system when
> we can do so easily.

OK, done.

> > +security {
> > +  require_message_authenticator = "yes"
> > +}
>
> > +# Note that require_message_authenticator defaulted to "no" before 3.2.5, 
> > and
> > +# then switched to "auto" (a new mode that fills the logs up with warning
> > +# messages about clients that don't send MA), and presumably a later 
> > version
> > +# will default to "yes".
>
> That's not quite accurate: the option didn't exist before version 3.2.5.
> What happens if you set it on an older server version? /me tests: seems
> that FreeRadius 3.2.1 silently accepts the setting, or any other setting
> it doesn't recognize, and will do nothing with it. A little surprising,
> but ok. I didn't find any mention in the docs on that.

Huh.  Thanks, I was confused by that.  Fixed.

> (Also, that will make the test fail, until the
> v1-0003-Mitigation-for-BlastRADIUS.patch is also applied. You could
> leave that out of the test in this first patch, and add it
> v1-0003-Mitigation-for-BlastRADIUS.patch)

Yeah, done.

> Review on v1-0003-Mitigation-for-BlastRADIUS.patch:
>
> > +  
> > +   radiusrequirema
> > +   
> > +
> > + Whether to require a valid 
> > Message-Authenticator
> > + attribute in messages received from RADIUS servers, and ignore 
> > messages
> > + that don't contain it.  The default value
> > + is 0, but it can be set to 1
> > + to enable that requirement.
> > + This setting does not affect requests sent by
> > + PostgreSQL to the RADIUS server, which
> > + always include a Message-Authenticator 
> > attribute
> > + (but didn't in earlier releases).
> > +
> > +   
> > +  
>
> I think this should include some advise on why and when you should set
> it. Something like:
>
> Enabling this mitigates the RADIUS protocol vulnerability described at
> blastradius.fail. It is recommended to always set this to 1, unless you
> are running an older RADIUS server version that does include the
> mitigation to include Message-Authenticator in all replies. The default
> will be changed to 1 in a future PostgreSQL version.

Done, with small tweaks.

> >   attr = (radius_attribute *) (packet + RADIUS_HEADER_LENGTH);
> >
> >   while ((uint8 *) attr < end)
> >   {
> >   /* Would this attribute overflow the buffer? */
> >   if (&attr->length >= end || (uint8 *) attr + attr->length > 
> > end)
> >   return false;
>
> Is this kind of pointer arithmetic safe? In theory, if a packet is
> malformed so that attr->length points beyond the end of the packet,
> "(uint8 *) attr + attr-length" might overflow. That would require the
> packet buffer to be allocated just before the end of the address space,
> so probably cannot happen in practice. I don't remember if there are
> some guarantees on that in C99 or other standards. Still, perhaps it
> would be better to write this differently, e.g. using a separate "size_t
> position" variable to track the current position in the buffer.

Done.

> (This also relies on the fact that "struct radius_attribute" doesn't
> require alignment, which is valid, and radius_add_attribute() made that
> assumption already. Maybe worth a comment though while we're at it; it
> certainly raised my eyebrow while reading this)

Comment added.

> What if the message contains multiple attribute of the same type? If
> there's a duplicate Message-Authenticator, we should surely reject the
> packet. I don't know if duplicate attributes are legal in general.

Duplicate attributes are legal in general per RFC 2865, which has a
table of attributes and their possible quantity; unfortunately this
one is an extension from RFC 2869, and I didn't find where it pins
that down.  I suppose we could try to detect an unexpected duplicate,
which might have the side benefit of checking the rest of the
attributes for well-formedness (though in practice there aren't any).
Is it worth bothering with that?

I suppose if we wanted to be extra fastidious, we could also test with
a gallery of malformed packets crafted by a Perl script, but that
feels like overkill.  On the other hand it would be bad if you could
crash a server by lobbing UDP packets at it because of some dumb
th

Re: Make tuple deformation faster

[David Rowley]

On Tue, 16 Jul 2024 at 00:13, Matthias van de Meent
 wrote:
>
> On Tue, 2 Jul 2024 at 02:23, David Rowley  wrote:
> > I'm happy to keep going with this version of the patch
>
> +1, go for it.

I've attached an updated patch series which are a bit more polished
than the last set. I've attempted to document the distinction between
FormData_pg_attribute and the abbreviated struct and tried to give an
indication of which one should be used.

Apart from that, changes include:

* I pushed the v1-0001 patch, so that's removed from the patch series.
* Rename TupleDescDeformAttr struct. It's now called CompactAttribute.
* Rename TupleDescDeformAttr() macro. It's now called TupleDescCompactAttr()
* Other macro renaming. e.g. ATT_IS_PACKABLE_FAST to COMPACT_ATTR_IS_PACKABLE
* In 0003, renamed CompactAttribute.attalign to attalignby to make it
easier to understand the distinction between the align char and the
number of bytes.
* Added 0004 patch to remove pg_attribute.attcacheoff.

There are a few more things that could be done to optimise a few more
things. For example, a bunch of places still use att_align_nominal().
With a bit more work, these could use att_nominal_alignby(). I'm not
yet sure of the cleanest way to do this. Adding alignby to the
typecache might be one way, or maybe just a function that converts the
attalign to the number of bytes. This would be useful in all places
where att_align_nominal() is used in loops, as converting the char to
the number of bytes would only be done once rather than once per loop.
I feel like this patch series is probably big enough for now, so I'd
like to opt for those improvements to take place as follow-on work.

I'll put this up for the CF bot to run with for a bit as the patch has
needed a rebase since I pushed the v1-0001 patch.

David


v2-0001-Move-TupleDesc.attrs-out-of-line.patch
Description: Binary data


v2-0002-Introduce-CompactAttribute-array-in-TupleDesc.patch
Description: Binary data


v2-0003-Optimize-alignment-calculations-in-tuple-form-def.patch
Description: Binary data


v2-0004-Remove-pg_attribute.attcacheoff-column.patch
Description: Binary data

Missing reflection in nls.mk from recent basebackup changes

[Kyotaro Horiguchi]

Hello.

After a recent commit f80b09bac8, make update-po fails with "missing
required files". The commit moved some files to fe_utils but this
change was not reflected in pg_basebackup's nls.mk. The attached patch
fixes this issue.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From f9eac9605d0309d32d6d1fab7a0a2d4e8d42798a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Tue, 6 Aug 2024 10:04:54 +0900
Subject: [PATCH] Fix nls.mk to reflect astreamer files relocation

In the recent commit f80b09bac8, astreamer files were moved to another
directory, but this change was not reflected in nls.mk. This commit
corrects that oversight.
---
 src/bin/pg_basebackup/nls.mk | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/bin/pg_basebackup/nls.mk b/src/bin/pg_basebackup/nls.mk
index 950b9797b1..f75f44dbe6 100644
--- a/src/bin/pg_basebackup/nls.mk
+++ b/src/bin/pg_basebackup/nls.mk
@@ -1,12 +1,7 @@
 # src/bin/pg_basebackup/nls.mk
 CATALOG_NAME = pg_basebackup
 GETTEXT_FILES= $(FRONTEND_COMMON_GETTEXT_FILES) \
-   astreamer_file.c \
-   astreamer_gzip.c \
astreamer_inject.c \
-   astreamer_lz4.c \
-   astreamer_tar.c \
-   astreamer_zstd.c \
pg_basebackup.c \
pg_createsubscriber.c \
pg_receivewal.c \
@@ -19,6 +14,11 @@ GETTEXT_FILES= $(FRONTEND_COMMON_GETTEXT_FILES) \
../../common/fe_memutils.c \
../../common/file_utils.c \
../../common/restricted_token.c \
+   ../../fe_utils/astreamer_file.c \
+   ../../fe_utils/astreamer_gzip.c \
+   ../../fe_utils/astreamer_lz4.c \
+   ../../fe_utils/astreamer_tar.c \
+   ../../fe_utils/astreamer_zstd.c \
../../fe_utils/option_utils.c \
../../fe_utils/recovery_gen.c \
../../fe_utils/string_utils.c
-- 
2.43.5

Re: Missing reflection in nls.mk from recent basebackup changes

[Michael Paquier]

On Tue, Aug 06, 2024 at 10:21:23AM +0900, Kyotaro Horiguchi wrote:
> After a recent commit f80b09bac8, make update-po fails with "missing
> required files". The commit moved some files to fe_utils but this
> change was not reflected in pg_basebackup's nls.mk. The attached patch
> fixes this issue.

You are right.  Still, it is not something that committers are
required to update when introducing new files, isn't it?  I don't see
why we should be aggressive here for HEAD.

ad8877cb5137 has done a large batch of these for the v17 cycle.
--
Michael


signature.asc
Description: PGP signature

Re: SQL:2011 application time

[jian he]

On Fri, Aug 2, 2024 at 1:09 AM Paul Jungwirth
 wrote:
>
> On 7/25/24 08:52, Paul Jungwirth wrote:
> > Here is a patch moving the not-empty check into 
> > check_exclusion_or_unique_constraint. That is a more
> > logical place for it than ExecConstraints, since WITHOUT OVERLAPS is part 
> > of the index constraint
> > (not a CHECK constraint). At that point we've already looked up all the 
> > information we need. So
> > there is no extra cost for non-temporal tables, and no need to change 
> > pg_class or add to the
> > relcache. Also putting it there means we don't need any extra code to 
> > enforce non-empties when we
> > build the index or do anything else with it.
> >
> > I think this is the nicest solution we can expect. It is even cleaner than 
> > the &&& ideas. So
> > hopefully this gets us back to where we were when we decided to commit PKs 
> > & FKs to v17.
> >
> > As before, I've left the nonempty check as a separate patch to make 
> > reviewing easier, but when
> > committing I would squash it with the PK patch.
>
> Hello,
>
> Here is an updated set of patches, rebased because the old patches no longer 
> applied.
>

void
ExecWithoutOverlapsNotEmpty(Relation rel, Datum attval, Oid typtype,
Oid atttypid);

should this just be a static function?
I am not so sure.

Oid typtype
should be
char typtype
?

 errmsg("new row for relation \"%s\" contains empty
WITHOUT OVERLAPS value",
we already have Form_pg_attribute via "TupleDesc tupdesc =
RelationGetDescr(heap);"
we can make the error message be:
 errmsg("cannot be empty range value for WITHOUT
OVERLAPS column \"%s\" in relation \"%s\", colname,
RelationGetRelationName(rel))


elog(ERROR, "Got unknown type for WITHOUT OVERLAPS column: %d", atttypid);
people will wonder if domain over range works or not. but currently
not, better error message would be:
elog(ERROR, "WITHOUT OVERLAPS column \"%s\" is not a range
or multirange type ", colname);
This part is unlikely to be reachable, so I don't have a strong opinion on it.


+ if (!found)
+ column = NULL;
this part no need?
because if not found, the column would be last element in ColumnDef
type list columns
also the following change also make sense:

+ if (!OidIsValid(typid) && column)
+ typid = typenameTypeId(NULL, column->typeName);


+ /* The WITHOUT OVERLAPS part (if any) must be a range or multirange type. */
+ if (constraint->without_overlaps && lc == list_last_cell(constraint->keys))
+ {
+ if (!found && cxt->isalter)
+ {
+ /*
+ * Look up the column type on existing table.
+ * If we can't find it, let things fail in DefineIndex.
+ */
+ Relation rel = cxt->rel;
+ for (int i = 0; i < rel->rd_att->natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(rel->rd_att, i);
+ const char *attname;
+
+ if (attr->attisdropped)
+ break;
+
+ attname = NameStr(attr->attname);
+ if (strcmp(attname, key) == 0)
+ {
+ typid = attr->atttypid;
+ break;
+ }
+ }
+ }
+ if (found)
+{
+}

I am confused with this change?
you found out the typid,but didn't using this information, should it be
+ if (strcmp(attname, key) == 0)
+ {
+ typid = attr->atttypid;
+ found = true;
+ break;
+ }

so the failing error message be same for the following two cases:
CREATE TABLE t1 (id int4range,valid_at tsrange,b text,
   CONSTRAINT temporal_rng_pk PRIMARY KEY (id, b WITHOUT OVERLAPS)
);

CREATE TABLE t1 (id int4range,valid_at tsrange,b text);
alter table t1 add CONSTRAINT temporal_rng_pk PRIMARY KEY (id, b
WITHOUT OVERLAPS);

Re: [HACKERS] make async slave to wait for lsn to be replayed

[Alexander Korotkov]

On Sat, Aug 3, 2024 at 6:07 AM Alexander Korotkov  wrote:
> On Sat, Aug 3, 2024 at 3:45 AM Kevin Hale Boyes  wrote:
> > In the for loop in WaitForLSNReplay, shouldn't the check for in-recovery be 
> > moved up above the call to GetXLogReplayRecPtr?
> > If we get promoted while waiting for the timeout we could call 
> > GetXLogReplayRecPtr while not in recovery.
>
> This is intentional.  After standby gets promoted,
> GetXLogReplayRecPtr() returns the last WAL position being replayed
> while being standby.  So, if standby reached target lsn before being
> promoted, we don't have to throw an error.
>
> But this gave me an idea that before the loop we probably need to put
> RecoveryInProgress() check after GetXLogReplayRecPtr() too.  I'll
> recheck that.

The attached patchset comprises assorted improvements for pg_wal_replay_wait().

The 0001 patch is intended to improve this situation.  Actually, it's
not right to just put RecoveryInProgress() after
GetXLogReplayRecPtr(), because more wal could be replayed between
these calls.  Instead we need to recheck GetXLogReplayRecPtr() after
getting negative result of RecoveryInProgress() because WAL replay
position couldn't get updated after.
0002 patch comprises fix for the header comment of WaitLSNSetLatches() function
0003 patch comprises tests for pg_wal_replay_wait() errors.

--
Regards,
Alexander Korotkov
Supabase


v1-0002-Improve-header-comment-for-WaitLSNSetLatches.patch
Description: Binary data


v1-0001-Adjust-pg_wal_replay_wait-procedure-behavior-on-p.patch
Description: Binary data


v1-0003-Add-tests-for-pg_wal_replay_wait-errors.patch
Description: Binary data

Re: Logical Replication of sequences

[shveta malik]

On Mon, Aug 5, 2024 at 5:28 PM Amit Kapila  wrote:
>
> On Mon, Aug 5, 2024 at 2:36 PM vignesh C  wrote:
> >
> > On Fri, 2 Aug 2024 at 14:24, shveta malik  wrote:
> > >
> > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik  
> > > wrote:
> > > >
> > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C  wrote:
> > > > >
> > > > > Thanks for reporting this, these issues are fixed in the attached
> > > > > v20240730_2 version patch.
> > > > >
> > >
> > > I was reviewing the design of patch003, and I have a query. Do we need
> > > to even start an apply worker and create replication slot when
> > > subscription created is for 'sequences only'? IIUC, currently logical
> > > replication apply worker is the one launching sequence-sync worker
> > > whenever needed. I think it should be the launcher doing this job and
> > > thus apply worker may even not be needed for current functionality of
> > > sequence sync?
> >
>
> But that would lead to maintaining all sequence-sync of each
> subscription by launcher. Say there are 100 sequences per subscription
> and some of them from each subscription are failing due to some
> reasons then the launcher will be responsible for ensuring all the
> sequences are synced. I think it would be better to handle
> per-subscription work by the apply worker.

I thought we can give that task to sequence-sync worker. Once sequence
sync worker is started by launcher, it keeps on syncing until all the
sequences are synced (even failed ones) and then exits only after all
are synced; instead of apply worker starting it multiple times for
failed sequences. Launcher to start sequence sync worker when signaled
by 'alter-sub refresh seq'.
But after going through details given by Vignesh in [1], I also see
the benefits of using apply worker for this task. Since apply worker
is already looping and doing that for table-sync, we can reuse the
same code for sequence sync and maintenance will be easy. So looks
okay if we go with existing apply worker design.

[1]: 
https://www.postgresql.org/message-id/CALDaNm1KO8f3Fj%2BRHHXM%3DUSGwOcW242M1jHee%3DX_chn2ToiCpw%40mail.gmail.com

>
> >
> > Going forward when we implement incremental sync of
> > > sequences, then we may have apply worker started but now it is not
> > > needed.
> >
> > I believe the current method of having the apply worker initiate the
> > sequence sync worker is advantageous for several reasons:
> > a) Reduces Launcher Load: This approach prevents overloading the
> > launcher, which must handle various other subscription requests.
> > b) Facilitates Incremental Sync: It provides a more straightforward
> > path to extend support for incremental sequence synchronization.
> > c) Reuses Existing Code: It leverages the existing tablesync worker
> > code for starting the tablesync process, avoiding the need to
> > duplicate code in the launcher.
> > d) Simplified Code Maintenance: Centralizing sequence synchronization
> > logic within the apply worker can simplify code maintenance and
> > updates, as changes will only need to be made in one place rather than
> > across multiple components.
> > e) Better Monitoring and Debugging: With sequence synchronization
> > being handled by the apply worker, you can more effectively monitor
> > and debug synchronization processes since all related operations are
> > managed by a single component.
> >
> > Also, I noticed that even when a publication has no tables, we create
> > replication slot and start apply worker.
> >
>
> As far as I understand slots and origins are primarily required for
> incremental sync. Would it be used only for sequence-sync cases? If
> not then we can avoid creating those. I agree that it would add some
> complexity to the code with sequence-specific checks, so we can create
> a top-up patch for this if required and evaluate its complexity versus
> the benefit it produces.
>
> --
> With Regards,
> Amit Kapila.

2024-08-08 update release announcement draft

[Jonathan S. Katz]

Hi,

Attached is the release announcement draft for the 2024-08-08 release. 
Please review for accuracy and omissions, and provide feedback no later 
than 2024-08-08 12:00 UTC.


Thanks!

Jonathan
The PostgreSQL Global Development Group has released an update to all supported
versions of PostgreSQL, including 16.4, 15.8, 14.13, 13.16, and 12.20, as well
as the third beta release of PostgreSQL 17. This release fixes over 55 bugs
reported over the last several months.

For the full list of changes, please review the
[release notes](https://www.postgresql.org/docs/release/).

PostgreSQL 12 EOL Notice


PostgreSQL 12 will stop receiving fixes on November 14, 2024. If you are
running PostgreSQL 12 in a production environment, we suggest that you make
plans to upgrade to a newer, supported version of PostgreSQL. Please see our
[versioning policy](https://www.postgresql.org/support/versioning/) for more
information.

Bug Fixes and Improvements
--
 
This update fixes over 55 bugs that were reported in the last several months.
The issues listed below affect PostgreSQL 16. Some of these issues may also
affect other supported versions of PostgreSQL.

* Avoid incorrect results from "Merge Right Anti Join" plans, where if the 
inner relation is known to have unique join keys, the merge could misbehave 
when there are duplicated join keys in the outer relation.
* Prevent infinite loop in 
[`VACUUM`](https://www.postgresql.org/docs/current/sql-vacuum.html).
* Fix partition pruning setup during [`ALTER TABLE DETACH ... PARTITION 
CONCURRENTLY`](https://www.postgresql.org/docs/current/sql-altertable.html).
* Fix behavior of stable functions that are used as an argument to a 
[`CALL`](https://www.postgresql.org/docs/current/sql-call.html) statement.
* `pg_sequence_last_value()` now returns `NULL` instead of throwing an error 
when called on unlogged sequences on standby servers and on temporary sequences 
of other sessions.
* Fix parsing of ignored operators in `websearch_to_tsquery()`.
* Correctly check updatability of view columns targeted by [`INSERT ... 
DEFAULT`](https://www.postgresql.org/docs/current/sql-insert.html).
* Lock owned sequences during [`ALTER TABLE ... SET 
LOGGED|UNLOGGED`](https://www.postgresql.org/docs/current/sql-altertable.html).
* Don't throw an error if a queued `AFTER` trigger no longer exists.
* Fix selection of an arbiter index for [`INSERT ... ON 
CONFLICT`](https://www.postgresql.org/docs/current/sql-insert.html) when the 
desired index has expressions or predicates, for example, through an updatable 
view.
* Refuse to modify a temporary table of another session with [`ALTER 
TABLE`](https://www.postgresql.org/docs/current/sql-altertable.html).
* Fix handling of extended statistics on expressions in [`CREATE TABLE ... LIKE 
STATISTICS`](https://www.postgresql.org/docs/current/sql-createtable.html).
* Fix failure to recalculate sub-queries generated from `MIN()` or `MAX()` 
aggregates.
* Disallow underscores in positional parameters.
* Avoid crashing when a JIT-inlined backend function throws an error.
* Fix handling of subtransactions of prepared transactions when starting a hot 
standby server.
* Prevent incorrect initialization of logical replication slots.
* Fix memory leak in the logical replication WAL sender when publishing changes 
to a partitioned table whose partitions have row types that are physically 
different from the table.
* Disable creation of stateful TLS session tickets by OpenSSL.
* Fix how [PL/pgSQL](https://www.postgresql.org/docs/current/plpgsql.html) 
handles integer ranges containing underscores (e.g., `FOR i IN 1_001..1_002`).
* Fix incompatibility between 
[PL/Perl](https://www.postgresql.org/docs/current/plperl.html) and Perl 5.40.
* Several fixes related to recursive 
[PL/Python](https://www.postgresql.org/docs/current/plpython.html) functions 
and triggers.
* Ensure that [`pg_restore 
-l`](https://www.postgresql.org/docs/current/app-pgrestore.html) reports 
dependent table of contents entries correctly.
* 
[`pg_stat_statements`](https://www.postgresql.org/docs/current/pgstatstatements.html)
 now passes a query ID for utility (non-`SELECT`/`INSERT`/`UPDATE`) statements 
that appears in SQL-language functions.
* Fix for 
[`postgres_fdw`](https://www.postgresql.org/docs/current/postgres-fdw.html) 
when mapping a foreign table to a nontrivial remote view.
* [`postgres_fdw`](https://www.postgresql.org/docs/current/postgres-fdw.html) 
no longer sends a `FETCH FIRST WITH TIES` clause to a remote server.

Updating


All PostgreSQL update releases are cumulative. As with other minor releases,
users are not required to dump and reload their database or use `pg_upgrade` in
order to apply this update release; you may simply shutdown PostgreSQL and
update its binaries.

Users who have skipped one or more update releases may need to run additional
post-update steps; please see the release notes from earlier versions for

Re: Logical Replication of sequences

[shveta malik]

On Tue, Aug 6, 2024 at 8:49 AM shveta malik  wrote:
>

Do we need some kind of coordination between table sync and sequence
sync for internally generated sequences? Lets say we have an identity
column with a 'GENERATED ALWAYS' sequence. When the sequence is synced
to subscriber, subscriber can also do an insert to table (extra one)
incrementing the sequence and then when publisher performs an insert,
apply worker will blindly copy that row to sub's table making identity
column's duplicate entries.

CREATE TABLE color ( color_id INT GENERATED ALWAYS AS
IDENTITY,color_name VARCHAR NOT NULL);

Pub: insert into color(color_name) values('red');

Sub: perform sequence refresh and check 'r' state is reached, then do insert:
insert into color(color_name) values('yellow');

Pub: insert into color(color_name) values('blue');

After above, data on Pub: (1, 'red') ;(2, 'blue'),

After above, data on Sub: (1, 'red') ;(2, 'yellow'); (2, 'blue'),

Identity column has duplicate values. Should the apply worker error
out while inserting such a  row to the table? Or it is not in the
scope of this project?

thanks
Shveta

Re: Fix memory counter update in reorderbuffer

[Amit Kapila]

On Sat, Aug 3, 2024 at 1:21 AM Masahiko Sawada  wrote:
>
> I found a bug in the memory counter update in reorderbuffer. It was
> introduced by commit 5bec1d6bc5e, so pg17 and master are affected.
>
> In ReorderBufferCleanupTXN() we zero the transaction size and then
> free the transaction entry as follows:
>
> /* Update the memory counter */
> ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
>
> /* deallocate */
> ReorderBufferReturnTXN(rb, txn);
>

Why do we need to zero the transaction size explicitly? Shouldn't it
automatically become zero after freeing all the changes?

> However, if the transaction entry has toast changes we free them in
> ReorderBufferToastReset() called from ReorderBufferToastReset(),
>

Here, you mean ReorderBufferToastReset() called from
ReorderBufferReturnTXN(), right?

BTW, commit 5bec1d6bc5e also introduced
ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
is also worth considering while fixing the reported problem. It may
not have the same problem as you have reported but we can consider
whether setting txn size as zero explicitly is required or not.

-- 
With Regards,
Amit Kapila.

Re: Logical Replication of sequences

[Amit Kapila]

On Tue, Aug 6, 2024 at 8:49 AM shveta malik  wrote:
>
> On Mon, Aug 5, 2024 at 5:28 PM Amit Kapila  wrote:
> >
> > On Mon, Aug 5, 2024 at 2:36 PM vignesh C  wrote:
> > >
> > > On Fri, 2 Aug 2024 at 14:24, shveta malik  wrote:
> > > >
> > > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik  
> > > > wrote:
> > > > >
> > > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C  wrote:
> > > > > >
> > > > > > Thanks for reporting this, these issues are fixed in the attached
> > > > > > v20240730_2 version patch.
> > > > > >
> > > >
> > > > I was reviewing the design of patch003, and I have a query. Do we need
> > > > to even start an apply worker and create replication slot when
> > > > subscription created is for 'sequences only'? IIUC, currently logical
> > > > replication apply worker is the one launching sequence-sync worker
> > > > whenever needed. I think it should be the launcher doing this job and
> > > > thus apply worker may even not be needed for current functionality of
> > > > sequence sync?
> > >
> >
> > But that would lead to maintaining all sequence-sync of each
> > subscription by launcher. Say there are 100 sequences per subscription
> > and some of them from each subscription are failing due to some
> > reasons then the launcher will be responsible for ensuring all the
> > sequences are synced. I think it would be better to handle
> > per-subscription work by the apply worker.
>
> I thought we can give that task to sequence-sync worker. Once sequence
> sync worker is started by launcher, it keeps on syncing until all the
> sequences are synced (even failed ones) and then exits only after all
> are synced; instead of apply worker starting it multiple times for
> failed sequences. Launcher to start sequence sync worker when signaled
> by 'alter-sub refresh seq'.
> But after going through details given by Vignesh in [1], I also see
> the benefits of using apply worker for this task. Since apply worker
> is already looping and doing that for table-sync, we can reuse the
> same code for sequence sync and maintenance will be easy. So looks
> okay if we go with existing apply worker design.
>

Fair enough. However, I was wondering whether apply_worker should exit
after syncing all sequences for a sequence-only subscription or should
it be there for future commands that can refresh the subscription and
add additional tables or sequences?

-- 
With Regards,
Amit Kapila.

Re: Logical Replication of sequences

[shveta malik]

On Tue, Aug 6, 2024 at 9:54 AM Amit Kapila  wrote:
>
> On Tue, Aug 6, 2024 at 8:49 AM shveta malik  wrote:
> >
> > > > > I was reviewing the design of patch003, and I have a query. Do we need
> > > > > to even start an apply worker and create replication slot when
> > > > > subscription created is for 'sequences only'? IIUC, currently logical
> > > > > replication apply worker is the one launching sequence-sync worker
> > > > > whenever needed. I think it should be the launcher doing this job and
> > > > > thus apply worker may even not be needed for current functionality of
> > > > > sequence sync?
> > > >
> > >
> > > But that would lead to maintaining all sequence-sync of each
> > > subscription by launcher. Say there are 100 sequences per subscription
> > > and some of them from each subscription are failing due to some
> > > reasons then the launcher will be responsible for ensuring all the
> > > sequences are synced. I think it would be better to handle
> > > per-subscription work by the apply worker.
> >
> > I thought we can give that task to sequence-sync worker. Once sequence
> > sync worker is started by launcher, it keeps on syncing until all the
> > sequences are synced (even failed ones) and then exits only after all
> > are synced; instead of apply worker starting it multiple times for
> > failed sequences. Launcher to start sequence sync worker when signaled
> > by 'alter-sub refresh seq'.
> > But after going through details given by Vignesh in [1], I also see
> > the benefits of using apply worker for this task. Since apply worker
> > is already looping and doing that for table-sync, we can reuse the
> > same code for sequence sync and maintenance will be easy. So looks
> > okay if we go with existing apply worker design.
> >
>
> Fair enough. However, I was wondering whether apply_worker should exit
> after syncing all sequences for a sequence-only subscription

If apply worker exits, then on next sequence-refresh, we need a way to
wake-up launcher to start apply worker which then will start
table-sync worker. Instead, won't it be better if the launcher starts
table-sync worker directly without the need of apply worker being
present (which I stated earlier).

>  or should
> it be there for future commands that can refresh the subscription and
> add additional tables or sequences?

If we stick with apply worker starting table sync worker when needed
by continuously checking seq-sync states ('i'/'r'), then IMO, it is
better that apply-worker stays. But if we want  apply-worker to exit
and start only when needed, then why not to start sequence-sync worker
directly for seq-only subscriptions?

thanks
Shveta

Re: [HACKERS] make async slave to wait for lsn to be replayed

[Michael Paquier]

On Tue, Aug 06, 2024 at 05:17:10AM +0300, Alexander Korotkov wrote:
> The 0001 patch is intended to improve this situation.  Actually, it's
> not right to just put RecoveryInProgress() after
> GetXLogReplayRecPtr(), because more wal could be replayed between
> these calls.  Instead we need to recheck GetXLogReplayRecPtr() after
> getting negative result of RecoveryInProgress() because WAL replay
> position couldn't get updated after.
> 0002 patch comprises fix for the header comment of WaitLSNSetLatches() 
> function
> 0003 patch comprises tests for pg_wal_replay_wait() errors.

Before adding more tests, could it be possible to stabilize what's in
the tree?  drongo has reported one failure with the recovery test
043_wal_replay_wait.pl introduced recently by 3c5db1d6b016:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2024-08-05%2004%3A24%3A54
--
Michael


signature.asc
Description: PGP signature

Re: Remove support for old realpath() API

[Michael Paquier]

On Mon, Aug 05, 2024 at 10:08:04AM -0400, Tom Lane wrote:
> Heikki Linnakangas  writes:
>> We don't seem to have any mentions of POSIX or SuS in docs, in the 
>> installation sections. There are a few mentions of POSIX-1.2008 and 
>> POSIX-1.2001 it in the commit log, though, where we require features 
>> specified by those. Can we rely on everything from POSIX-1-2008 
>> nowadays, or is it more on a case-by-case basis, depending on which 
>> parts of POSIX are supported by various platforms?
> 
> I'd say it's still case-by-case.  Perhaps everything in POSIX-1.2008
> is supported now on every platform we care about, but perhaps not.

Just pointing at the message where this has been discussed previously,
for reference:
https://www.postgresql.org/message-id/1457809.1662232...@sss.pgh.pa.us

Leaving Solaris aside because there is nothing older than 11 in the
buildfarm currently, I am dubious that it is a good idea to remove
this code knowing that we have a thread from a few months ago about
the fact that we have folks complaining about AIX support and that we
should bring it back:
https://www.postgresql.org/message-id/cy5pr11mb63928cc05906f27fb10d74d0fd...@cy5pr11mb6392.namprd11.prod.outlook.com
--
Michael


signature.asc
Description: PGP signature

Re: Do we still need parent column in pg_backend_memory_context?

[David Rowley]

On Wed, 31 Jul 2024 at 13:27, Tom Lane  wrote:
> David Rowley  writes:
> > One thing we could do is remove it and see if anyone complains.  If we
> > did that today, there's about a year-long window for people to
> > complain where we could still reverse the decision.
>
> Seems like a plausible compromise.

Does anyone object to making this happen? i.e. remove
pg_backend_memory_contexts.parent column and see if anyone complains?

If nobody comes up with any reasons against it, then I propose making
this happen.

David

Instability with incremental backup tests (pg_combinebackup, 003_timeline.pl)

[Michael Paquier]

Hi all,

dikkop has reported a failure with the regression tests of pg_combinebackup:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dikkop&dt=2024-08-04%2010%3A04%3A51

That's in the test 003_timeline.pl, from dc212340058b:
#   Failed test 'incremental backup from node1'
#   at t/003_timeline.pl line 43.

The node is extremely slow, so perhaps bumping up the timeout would be
fine enough in this case (did not spend time analyzing it).  I don't
think that this has been discussed, but perhaps I just missed a
reference to it and the incremental backup thread is quite large.

Thanks,
--
Michael


signature.asc
Description: PGP signature

Re: pg_verifybackup: TAR format backup verification

[Amul Sul]

On Mon, Aug 5, 2024 at 10:29 PM Robert Haas  wrote:
>
> On Fri, Aug 2, 2024 at 7:43 AM Amul Sul  wrote:
> > Please consider the attached version for the review.
>
> Thanks. I committed 0001-0003. The only thing that I changed was that
> in 0001, you forgot to pgindent, which actually mattered quite a bit,
> because astreamer is one character shorter than bbstreamer.
>

Understood. Thanks for tidying up and committing the patches.

> Before we proceed with the rest of this patch series, I think we
> should fix up the comments for some of the astreamer files. Proposed
> patch for that attached; please review.
>

Looks good to me, except for the following typo that I have fixed in
the attached version:

s/astreamer_plan_writer/astreamer_plain_writer/

> I also noticed that cfbot was unhappy about this patch set:
>
> [10:37:55.075] pg_verifybackup.c:100:7: error: no previous extern
> declaration for non-static variable 'format'
> [-Werror,-Wmissing-variable-declarations]
> [10:37:55.075] charformat = '\0';  /* p(lain)/t(ar) */
> [10:37:55.075] ^
> [10:37:55.075] pg_verifybackup.c:100:1: note: declare 'static' if the
> variable is not intended to be used outside of this translation unit
> [10:37:55.075] charformat = '\0';  /* p(lain)/t(ar) */
> [10:37:55.075] ^
> [10:37:55.075] pg_verifybackup.c:101:23: error: no previous extern
> declaration for non-static variable 'compress_algorithm'
> [-Werror,-Wmissing-variable-declarations]
> [10:37:55.075] pg_compress_algorithm compress_algorithm = PG_COMPRESSION_NONE;
> [10:37:55.075]   ^
> [10:37:55.075] pg_verifybackup.c:101:1: note: declare 'static' if the
> variable is not intended to be used outside of this translation unit
> [10:37:55.075] pg_compress_algorithm compress_algorithm = PG_COMPRESSION_NONE;
> [10:37:55.075] ^
> [10:37:55.075] 2 errors generated.
>

Fixed in the attached version.

> Please fix and, after posting future versions of the patch set, try to
> remember to check http://cfbot.cputube.org/amul-sul.html

Sure. I used to rely on that earlier, but after Cirrus CI in the
GitHub repo, I assumed the workflow would be the same as cfbot and
started overlooking it. However, cfbot reported a warning that didn't
appear in my GitHub run. From now on, I'll make sure to check cfbot as
well.

Regards,
Amul
From 975b994d8ca76f3c16b7eb9119e45ec25dba410b Mon Sep 17 00:00:00 2001
From: Amul Sul 
Date: Tue, 2 Jul 2024 10:26:35 +0530
Subject: [PATCH v7 10/12] pg_verifybackup: Add backup format and compression
 option


NOTE: This patch is not meant to be committed separately. It should
be squashed with the next patch that implements tar format support.

---
 src/bin/pg_verifybackup/pg_verifybackup.c | 146 +-
 1 file changed, 144 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index 801e13886c2..f20b6e2895c 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 
+#include "common/compression.h"
 #include "common/parse_manifest.h"
 #include "fe_utils/simple_list.h"
 #include "getopt_long.h"
@@ -56,6 +57,8 @@ static void report_manifest_error(JsonManifestParseContext *context,
   const char *fmt,...)
 			pg_attribute_printf(2, 3) pg_attribute_noreturn();
 
+static char find_backup_format(verifier_context *context);
+static pg_compress_algorithm find_backup_compression(verifier_context *context);
 static void verify_backup_directory(verifier_context *context,
 	char *relpath, char *fullpath);
 static void verify_backup_file(verifier_context *context,
@@ -74,6 +77,9 @@ static void usage(void);
 
 static const char *progname;
 
+static char format = '\0';		/* p(lain)/t(ar) */
+static pg_compress_algorithm compress_algorithm = PG_COMPRESSION_NONE;
+
 /*
  * Main entry point.
  */
@@ -84,11 +90,13 @@ main(int argc, char **argv)
 		{"exit-on-error", no_argument, NULL, 'e'},
 		{"ignore", required_argument, NULL, 'i'},
 		{"manifest-path", required_argument, NULL, 'm'},
+		{"format", required_argument, NULL, 'F'},
 		{"no-parse-wal", no_argument, NULL, 'n'},
 		{"progress", no_argument, NULL, 'P'},
 		{"quiet", no_argument, NULL, 'q'},
 		{"skip-checksums", no_argument, NULL, 's'},
 		{"wal-directory", required_argument, NULL, 'w'},
+		{"compress", required_argument, NULL, 'Z'},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -99,6 +107,7 @@ main(int argc, char **argv)
 	bool		quiet = false;
 	char	   *wal_directory = NULL;
 	char	   *pg_waldump_path = NULL;
+	bool		tar_compression_specified = false;
 
 	pg_logging_init(argv[0]);
 	set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_verifybackup"));
@@ -141,7 +150,7 @@ main(int argc, char **argv)
 	simple_string_list_append(&context.ignore_list, "recovery.signal");
 	simple_string_list_append(&context.ignore_list, "standby.signal");
 
-	while ((c = getopt_lo

Re: BlastRADIUS mitigation

[Michael Paquier]

On Mon, Aug 05, 2024 at 05:41:21PM +0300, Heikki Linnakangas wrote:
> On 05/08/2024 15:43, Thomas Munro wrote:
>> Since PostgreSQL v12 and v13 don't have the modern "common/hmac.h"
>> API, I came up with a cheap kludge: locally #define those interfaces
>> to point directly to the OpenSSL HMAC API, or just give up and drop
>> Message-Authenticator support if you didn't build with OpenSSL support
>> (in practice everyone does).  Better ideas?
> 
> Seems reasonable. It probably wouldn't be hard to backport common/hmac.h
> either, perhaps in a limited fashion with just md5 support.

It's a bit more than just backporting hmac.h and hmac.c.
hmac_openssl.c only depends on OpenSSL to do its business, but the 
non-OpenSSL fallback implementation depends also on the cryptohash
fallbacks for SHA-NNN and MD5.  So you would also need the parts
related to cryptohash.c, sha{1,2}.c, etc.  Not really complex as these
could be dropped as-is into the stable branches of 12 and 13, but not
that straight-forward either as we had the bad idea to use the
fallback MD5 implementation even if linking to OpenSSL in v12 and v13,
meaning that you may need some tweaks to avoid API conflicts.

Requiring OpenSSL and its HMAC APIs to do the job is much safer for a
stable branch, IMO.
--
Michael


signature.asc
Description: PGP signature

Fsync (flush) all inserted WAL records

[Vitaly Davydov]

Hi Hackers,

I use async commits. At some moment, I would like to make sure that all 
inserted WAL records are fsync-ed. I can use XLogFlush function but I have some 
doubts which LSN to specify. There is a number of functions which return write 
or insert LSNs but they are not applicable.

I can't use GetXLogInsertRecPtr() because it returns a real insert LSN, not the 
end LSN of the last record. XLogFlush may fail with such LSN because the 
specified LSN may be "in the future" if the WAL record ends up to the page 
boundary (the real insert LSN is summed up with page header size).

I can't use GetXLogWriteRecPtr() because it seems to be bounded to page 
boundaries. Some inserted WAL records may not be fsync-ed. Some other functions 
seems not applicable as well.

The first idea is to use GetLastImportantRecPtr() but this function returns the 
start LSN of the last important WAL record. I would use 
XLogFlush(GetLastImportantRecPtr() + 1) but I'm not sure that this way is 
conventional.

Another idea is to create a new function like GetXLogInsertRecPtr() which calls 
XLogBytePosToEndRecPtr() instead of XLogBytePosToRecPtr() inside it.

Could you please advice which way to go?

With best regards,
Vitaly