Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Peter Eisentraut]

On 05.04.23 23:29, Jacob Champion wrote:

On Wed, Apr 5, 2023 at 2:27 PM Daniel Gustafsson  wrote:

I squashed and pushed v10 with a few small comment tweaks for typos and some
pgindenting. Thanks!


Thank you very much!


This patch (8eda731465) makes the ssl tests fail for me:

not ok 121 - sslrootcert=system does not connect with private CA: matches

#   Failed test 'sslrootcert=system does not connect with private CA: 
matches'

#   at t/001_ssltests.pl line 479.
#   'psql: error: connection to server at "127.0.0.1", 
port 53971 failed: SSL SYSCALL error: Undefined error: 0'

# doesn't match '(?^:SSL error: certificate verify failed)'

This is with OpenSSL 3.1.0 from macOS/Homebrew.

If I instead use OpenSSL 1.1.1t, then the tests pass.

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Daniel Gustafsson]

> On 12 Apr 2023, at 09:11, Peter Eisentraut 
>  wrote:
> 
> On 05.04.23 23:29, Jacob Champion wrote:
>> On Wed, Apr 5, 2023 at 2:27 PM Daniel Gustafsson  wrote:
>>> I squashed and pushed v10 with a few small comment tweaks for typos and some
>>> pgindenting. Thanks!
>> Thank you very much!
> 
> This patch (8eda731465) makes the ssl tests fail for me:
> 
> not ok 121 - sslrootcert=system does not connect with private CA: matches
> 
> #   Failed test 'sslrootcert=system does not connect with private CA: matches'
> #   at t/001_ssltests.pl line 479.
> #   'psql: error: connection to server at "127.0.0.1", port 
> 53971 failed: SSL SYSCALL error: Undefined error: 0'
> # doesn't match '(?^:SSL error: certificate verify failed)'
> 
> This is with OpenSSL 3.1.0 from macOS/Homebrew.
> 
> If I instead use OpenSSL 1.1.1t, then the tests pass.

Thanks for the report, I'll look at it today as an open item for 16.

--
Daniel Gustafsson

Re: Direct I/O

[Thomas Munro]

On Wed, Apr 12, 2023 at 5:48 PM Thomas Munro  wrote:
> On Wed, Apr 12, 2023 at 3:04 PM Thomas Munro  wrote:
> > On Wed, Apr 12, 2023 at 2:56 PM Christoph Berg  wrote:
> > > I'm hitting a panic in t_004_io_direct. The build is running on
> > > overlayfs on tmpfs/ext4 (upper/lower) which is probably a weird
> > > combination but has worked well for building everything over the last
> > > decade. On Debian unstable:

After trying a couple of things and doing some googling, it looks like
it's tmpfs that rejects it, not overlayfs, so I'd adjust that commit
message slightly.  Of course it's a completely reasonable thing to
expect the tests to pass (or in this case be skipped) in a tmpfs, eg
/tmp on some distributions.  (It's a strange to contemplate what
O_DIRECT means for tmpfs, considering that it *is* the page cache,
kinda, and I see people have been arguing about that for a couple of
decades since O_DIRECT was added to Linux; doesn't seem that helpful
to me that it rejects it, but 🤷).

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear Peter,

Thank you for giving comments. PSA new version.

> src/bin/pg_dump/pg_backup.h
> 
> 1.
> + int logical_slot_only;
> 
> The field should be plural - "logical_slots_only"

Fixed.

> src/bin/pg_dump/pg_dump.c
> 
> 2.
> + appendPQExpBufferStr(query,
> + "SELECT r.slot_name, r.plugin, r.two_phase "
> + "FROM pg_replication_slots r "
> + "WHERE r.database = current_database() AND temporary = false "
> + "AND wal_status IN ('reserved', 'extended');");
> 
> The alias 'r' may not be needed at all here, but since you already
> have it IMO it looks a bit strange that you used it for only some of
> the columns but not others.

Right, I removed alias. Moreover, the namespace 'pg_catalog' is now specified.

> 3.
> +
> + /* FIXME: force dumping */
> + slotinfo[i].dobj.dump = DUMP_COMPONENT_ALL;
> 
> Why the "FIXME" here? Are you intending to replace this code with
> something else?

I was added FIXME because I was not sure whether we must add selectDumpable...()
function was needed or not. Now I have been thinking that such a functions are 
not
needed, so replaced comments. More detail, please see following:

Replication slots cannot be a member of extension because 
pg_create_logical_replication_slot()
cannot be called within the install script. This means that 
checkExtensionMembership()
is not needed. Moreover, we do not have have any options to include/exclude 
slots
in dumping, so checking their name like selectDumpableExtension() is not needed.
Based on them, I think the function is not needed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED



v5-0001-pg_upgrade-Add-include-logical-replication-slots-.patch
Description:  v5-0001-pg_upgrade-Add-include-logical-replication-slots-.patch

Re: When to drop src/tools/msvc support

[Peter Eisentraut]

On 08.04.23 21:10, Andres Freund wrote:

Do we want to drop src/tools/msvc support in 16 (i.e. now), or do it early in
17?


Can you build using meson from a distribution tarball on Windows?

Fix incorrect start up costs for WindowAgg paths (bug #17862)

[David Rowley]

Over on [1], Tim reported that the planner is making some bad choices
when the plan contains a WindowFunc which requires reading all of, or
a large portion of the WindowAgg subnode in order to produce the first
WindowAgg row.

For example:

EXPLAIN (ANALYZE, TIMING OFF)
SELECT COUNT(*) OVER ()
FROM tenk1 t1 INNER JOIN tenk1 t2 ON t1.unique1 = t2.tenthous
LIMIT 1;

With master, we get the following plan:

QUERY PLAN
---
 Limit  (cost=0.29..0.67 rows=1 width=8) (actual time=47.491..47.492
rows=1 loops=1)
   ->  WindowAgg  (cost=0.29..3815.00 rows=1 width=8) (actual
time=47.489..47.490 rows=1 loops=1)
 ->  Nested Loop  (cost=0.29..3690.00 rows=1 width=0)
(actual time=0.026..42.972 rows=1 loops=1)
   ->  Seq Scan on tenk1 t2  (cost=0.00..445.00 rows=1
width=4) (actual time=0.009..1.734 rows=1 loops=1)
   ->  Index Only Scan using tenk1_unique1 on tenk1 t1
(cost=0.29..0.31 rows=1 width=4) (actual time=0.003..0.004 rows=1
loops=1)
 Index Cond: (unique1 = t2.tenthous)
 Heap Fetches: 0
 Planning Time: 0.420 ms
 Execution Time: 48.107 ms

You can see that the time to get the first WindowAgg row (47.489 ms)
is not well aligned to the startup cost (0.29).  This effectively
causes the planner to choose a Nested Loop plan as it thinks it'll
read just 1 row from the join.  Due to the OVER (), we'll read all
rows! Not good.

It's not hard to imagine that a slightly different schema could yield
a *far* worse plan if it opted to use a non-parameterised nested loop
plan and proceed to read all rows from it.

With the attached patch, that turns into:


   QUERY PLAN
--
 Limit  (cost=928.02..928.02 rows=1 width=8) (actual
time=29.308..29.310 rows=1 loops=1)
   ->  WindowAgg  (cost=928.02..928.07 rows=1 width=8) (actual
time=29.306..29.308 rows=1 loops=1)
 ->  Hash Join  (cost=395.57..803.07 rows=1 width=0)
(actual time=10.674..22.032 rows=1 loops=1)
   Hash Cond: (t1.unique1 = t2.tenthous)
   ->  Index Only Scan using tenk1_unique1 on tenk1 t1
(cost=0.29..270.29 rows=1 width=4) (actual time=0.036..4.961
rows=1 loops=1)
 Heap Fetches: 0
   ->  Hash  (cost=270.29..270.29 rows=1 width=4)
(actual time=10.581..10.582 rows=1 loops=1)
 Buckets: 16384  Batches: 1  Memory Usage: 480kB
 ->  Index Only Scan using tenk1_thous_tenthous on
tenk1 t2  (cost=0.29..270.29 rows=1 width=4) (actual
time=0.055..5.437 rows=1 loops=1)
   Heap Fetches: 0
 Planning Time: 2.415 ms
 Execution Time: 30.554 ms


I'm not sure if we should consider backpatching a fix for this bug.
We tend not to commit stuff that would destabilise plans in the back
branches.  On the other hand, it's fairly hard to imagine  how we
could make this much worse even given bad estimates.

I do think we should fix this in v16, however.

I'll add this to the "Older bugs affecting stable branches" section of
the PG 16 open items list

David

[1] https://postgr.es/m/17862-1ab8f74b0f7b0...@postgresql.org


fix_bug_17862.patch
Description: Binary data

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Daniel Gustafsson]

> On 12 Apr 2023, at 09:11, Peter Eisentraut 
>  wrote:
> 
> On 05.04.23 23:29, Jacob Champion wrote:
>> On Wed, Apr 5, 2023 at 2:27 PM Daniel Gustafsson  wrote:
>>> I squashed and pushed v10 with a few small comment tweaks for typos and some
>>> pgindenting. Thanks!
>> Thank you very much!
> 
> This patch (8eda731465) makes the ssl tests fail for me:
> 
> not ok 121 - sslrootcert=system does not connect with private CA: matches
> 
> #   Failed test 'sslrootcert=system does not connect with private CA: matches'
> #   at t/001_ssltests.pl line 479.
> #   'psql: error: connection to server at "127.0.0.1", port 
> 53971 failed: SSL SYSCALL error: Undefined error: 0'
> # doesn't match '(?^:SSL error: certificate verify failed)'
> 
> This is with OpenSSL 3.1.0 from macOS/Homebrew.
> 
> If I instead use OpenSSL 1.1.1t, then the tests pass.

I am unable to reproduce this (or any failure) with OpenSSL 3.1 built from
source (or 3.0 or 3.1.1-dev) or installed via homebrew (on macOS 12 with Intel
CPU).  Do you have any more clues from logs what might've happened?

--
Daniel Gustafsson

RE: pg_upgrade and logical replication

[Hayato Kuroda (Fujitsu)]

Dear Julien,

Thank you for updating the patch. I checked yours.
Followings are general or non-minor questions:

1.
Feature freeze for PG16 has already come. So I think there is no reason to rush
making the patch. Based on above, could you allow to upgrade while synchronizing
data? Personally it can be added as 0002 patch which extends the feature. Or
have you already found any problem?

2.
I have a questions about the SQL interface:

ALTER SUBSCRIPTION name ADD TABLE (relid = XYZ, state = 'x' [, lsn = 'X/Y'])

Here the oid of the table is directly specified, but is it really kept between
old and new node? Similar command ALTER PUBLICATION requires the name of table,
not the oid.

3.
Currently getSubscriptionRels() is called from the getSubscriptions(), but I 
could
not find the reason why we must do like that. Other functions like
getPublicationTables() is directly called from getSchemaData(), so they should
be followed. Additionaly, I found two problems.

* Only tables that to be dumped should be included. See getPublicationTables().
* dropStmt for subscription relations seems not to be needed.
* Maybe security label and comments should be also dumped.

Followings are minor comments.


4. parse_subscription_options

```
+   opts->state = defGetString(defel)[0];
```

[0] is not needed.

5. AlterSubscription

```
+   supported_opts = SUBOPT_RELID | SUBOPT_STATE | 
SUBOPT_LSN;
+   parse_subscription_options(pstate, 
stmt->options,
+   
   supported_opts, &opts);
+
+   /* relid and state should always be provided. */
+   Assert(IsSet(opts.specified_opts, 
SUBOPT_RELID));
+   Assert(IsSet(opts.specified_opts, 
SUBOPT_STATE));
+
```

SUBOPT_LSN accepts "none" string, which means InvalidLSN. Isn't it better to
reject it?

6. dumpSubscription()

```
+   if (dopt->binary_upgrade && dopt->preserve_subscriptions &&
+   subinfo->suboriginremotelsn)
+   {
+   appendPQExpBuffer(query, ", lsn = '%s'", 
subinfo->suboriginremotelsn);
+   }
```

{} is not needed.

7. pg_dump.h

```
+/*
+ * The SubRelInfo struct is used to represent subscription relation.
+ */
+typedef struct _SubRelInfo
+{
+   Oid srrelid;
+   charsrsubstate;
+   char   *srsublsn;
+} SubRelInfo;
```

This typedef must be added to typedefs.list.

8. check_for_subscription_state

```
nb = atooid(PQgetvalue(res, 0, 0));
if (nb != 0)
{
is_error = true;
pg_log(PG_WARNING,
   "\nWARNING:  %d subscription have 
invalid remote_lsn",
   nb);
}
```

I think no need to use atooid. Additionaly, isn't it better to show the name of
subscriptions which have invalid remote_lsn?

```
nb = atooid(PQgetvalue(res, 0, 0));
if (nb != 0)
{
is_error = true;
pg_log(PG_WARNING,
   "\nWARNING: database \"%s\" has %d 
subscription "
   "relations(s) in non-ready state", 
active_db->db_name, nb);
}
```

Same as above.

9. parseCommandLine

```
+   user_opts.preserve_subscriptions = false;
```

I think this initialization is not needed because it is default.

And maybe you missed to run pgindent.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Unexpected (wrong?) result querying boolean partitioned table with NULL partition

[David Kimura]

Hi Hackers,

Is it fair to assume that, given the same data, a partitioned table should
return the same results as a non-partitioned table?  If that's true, then I
think I may have stumbled across a case of wrong results on boolean partitioned
tables.

In following example, I think we incorrectly skip the default partition scan:

CREATE TABLE boolpart (a bool) PARTITION BY LIST (a);
CREATE TABLE boolpart_default PARTITION OF boolpart default;
CREATE TABLE boolpart_t PARTITION OF boolpart FOR VALUES IN ('true');
CREATE TABLE boolpart_f PARTITION OF boolpart FOR VALUES IN ('false');
INSERT INTO boolpart VALUES (true), (false), (null);

EXPLAIN SELECT * FROM boolpart WHERE a IS NOT true;
  QUERY PLAN
---
 Seq Scan on boolpart_f boolpart  (cost=0.00..38.10 rows=1405 width=1)
   Filter: (a IS NOT TRUE)
(2 rows)

SELECT * FROM boolpart WHERE a IS NOT true;
 a
---
 f
(1 row)

Compare that to the result of a non-partitioned table:

CREATE TABLE booltab (a bool);
INSERT INTO booltab VALUES (true), (false), (null);

EXPLAIN SELECT * FROM booltab WHERE a IS NOT true;
QUERY PLAN
---
 Seq Scan on booltab  (cost=0.00..38.10 rows=1405 width=1)
   Filter: (a IS NOT TRUE)
(2 rows)

SELECT * FROM booltab WHERE a IS NOT true;
 a
---
 f

(2 rows)

I think the issue has to do with assumptions made about boolean test IS NOT
inequality logic which is different from inequality of other operators.
Specifically, "true IS NOT NULL" is not the same as "true<>NULL".

In partition pruning, match_boolean_partition_clause() tries to match partkey
with clause and outputs PARTCLAUSE_MATCH_CLAUSE and an outconst TRUE for
(IS_TRUE or IS_NOT_FALSE) and inversely FALSE for (IS_FALSE or IS_NOT_TRUE).
However, I don't think this gradularity is sufficient for "IS NOT" logic when a
NULL value partition is present.

One idea is to use the negation operator for IS_NOT_(true|false) (i.e.
BooleanNotEqualOperator instead of BooleanEqualOperator). But besides
presumably being a more expensive operation, not equal is not part of the btree
opfamily for bool_ops. So, seems like that won't really fit into the current
partition pruning framework.

Then I realized that the issue is just about adding the default or null
partition in these very particular scenarios. And struct PartitionBoundInfoData
already holds that information. So if we can identify these scenarios and pass
that information into get_matching_partitions() then we can add the necessary
partitions. Attached is a very rough sketch of that idea.

Thoughts? Does this seem like a legit issue? And if so, do either of the
proposed solutions seem reasonable?

Thanks,
David


boolpart_scan_nullpart.patch
Description: Binary data

Re: Unexpected (wrong?) result querying boolean partitioned table with NULL partition

[David Rowley]

On Wed, 12 Apr 2023 at 22:13, David Kimura  wrote:
> Is it fair to assume that, given the same data, a partitioned table should
> return the same results as a non-partitioned table?

Yes, and also the same as when enable_partition_pruning is set to off.

> CREATE TABLE boolpart (a bool) PARTITION BY LIST (a);
> CREATE TABLE boolpart_default PARTITION OF boolpart default;
> CREATE TABLE boolpart_t PARTITION OF boolpart FOR VALUES IN ('true');
> CREATE TABLE boolpart_f PARTITION OF boolpart FOR VALUES IN ('false');
> INSERT INTO boolpart VALUES (true), (false), (null);
>
> EXPLAIN SELECT * FROM boolpart WHERE a IS NOT true;
>   QUERY PLAN
> ---
>  Seq Scan on boolpart_f boolpart  (cost=0.00..38.10 rows=1405 width=1)
>Filter: (a IS NOT TRUE)
> (2 rows)
>
> SELECT * FROM boolpart WHERE a IS NOT true;
>  a
> ---
>  f
> (1 row)
>
> Compare that to the result of a non-partitioned table:
>
> CREATE TABLE booltab (a bool);
> INSERT INTO booltab VALUES (true), (false), (null);
>
> EXPLAIN SELECT * FROM booltab WHERE a IS NOT true;
> QUERY PLAN
> ---
>  Seq Scan on booltab  (cost=0.00..38.10 rows=1405 width=1)
>Filter: (a IS NOT TRUE)
> (2 rows)
>
> SELECT * FROM booltab WHERE a IS NOT true;
>  a
> ---
>  f

Ouch.  That's certainly not correct.

> I think the issue has to do with assumptions made about boolean test IS NOT
> inequality logic which is different from inequality of other operators.
> Specifically, "true IS NOT NULL" is not the same as "true<>NULL".

Yeah, that's wrong.

> One idea is to use the negation operator for IS_NOT_(true|false) (i.e.
> BooleanNotEqualOperator instead of BooleanEqualOperator). But besides
> presumably being a more expensive operation, not equal is not part of the 
> btree
> opfamily for bool_ops. So, seems like that won't really fit into the current
> partition pruning framework.

There's already code to effectively handle <> operators. Just the
PartClauseInfo.op_is_ne needs to be set to true.
get_matching_list_bounds() then handles that by taking the inverse of
the partitions matching the equality operator.

Effectively, I think that's the attached patch.

There seems to be a bunch of tests checking this already, all of them
assuming the incorrect plans.

David


fix_partprune_handling_of_NOT_TRUE_and_NOT_FALSE_boolean_quals.patch
Description: Binary data

Re: Support logical replication of DDLs

[Amit Kapila]

On Fri, Apr 7, 2023 at 8:52 AM houzj.f...@fujitsu.com
 wrote:
>

Few comments on 0001
===
1.
+ ConstrObjDomain,
+ ConstrObjForeignTable
+} ConstraintObjType;

These both object types don't seem to be supported by the first patch.
So, I don't see why these should be part of it.

2.
+append_string_object(ObjTree *tree, char *sub_fmt, char * object_name,

Extra space before object_name.

3. Is there a reason to keep format_type_detailed() in ddl_deparse.c
instead of defining it in format_type.c where other format functions
reside? Earlier, we were doing this deparsing as an extension, so it
makes sense to define it locally but not sure if that is required now.

4.
format_type_detailed()
{
...
+ /*
+ * Check if it's a regular (variable length) array type.  As above,
+ * fixed-length array types such as "name" shouldn't get deconstructed.
+ */
+ array_base_type = typeform->typelem;

This comment gives incomplete information. I think it is better to
say: "We switch our attention to the array element type for certain
cases. See format_type_extended(). Then we can remove a similar
comment later in the function.

5.
+
+ switch (type_oid)
+ {
+ case INTERVALOID:
+ *typename = pstrdup("INTERVAL");
+ break;
+ case TIMESTAMPTZOID:
+ if (typemod < 0)
+ *typename = pstrdup("TIMESTAMP WITH TIME ZONE");
+ else
+ /* otherwise, WITH TZ is added by typmod. */
+ *typename = pstrdup("TIMESTAMP");
+ break;
+ case TIMESTAMPOID:
+ *typename = pstrdup("TIMESTAMP");
+ break;

In this switch case, use the type oid cases in the order of their value.

6.
+static inline char *
+get_type_storage(char storagetype)

We already have a function with the name storage_name() which does
exactly what this function is doing. Shall we expose that and use it?

7.
+static ObjTree *
+new_objtree(char *fmt)
+{
+ ObjTree*params;
+
+ params = palloc0(sizeof(ObjTree));

Here, the variable name params appear a bit odd. Shall we change it to
objtree or obj?

-- 
With Regards,
Amit Kapila.

Wrong results from Parallel Hash Full Join

[Richard Guo]

I came across $subject and reduced the repro query as below.

create table a (i int);
create table b (i int);
insert into a values (1);
insert into b values (2);
update b set i = 2;

set min_parallel_table_scan_size to 0;
set parallel_tuple_cost to 0;
set parallel_setup_cost to 0;

# explain (costs off) select * from a full join b on a.i = b.i;
QUERY PLAN
--
 Gather
   Workers Planned: 2
   ->  Parallel Hash Full Join
 Hash Cond: (a.i = b.i)
 ->  Parallel Seq Scan on a
 ->  Parallel Hash
   ->  Parallel Seq Scan on b
(7 rows)

# select * from a full join b on a.i = b.i;
 i | i
---+---
 1 |
(1 row)

Tuple (NULL, 2) is missing from the results.

Thanks
Richard

Re: [BUG #17888] Incorrect memory access in gist__int_ops for an input array with many elements

[David Rowley]

On Wed, 12 Apr 2023 at 03:49, Ankit Kumar Pandey  wrote:
> Also, comments on BogusFree mentions `As a possible
> aid in debugging, we report the header word along with the pointer
> address`. How can we interpret useful debugging information from this?
>
> `pfree called with invalid pointer 0x7ff1706d0030 (header
> 0x4fc80001)`

elog(ERROR)s are not meant to happen.  ISTM, what's there is about the
best that can be done with our current infrastructure. If that occurs
on some machine that we can't get access to debug on, then having the
header bits might be useful, at least, certainly much more useful than
just not having them at all.

If you can think of something more useful to put in the elog, then we
could consider changing it to improve it.

Just in case you suggest it, I don't believe it's wise to try and
split it out into the components of MemoryChunk's hdrmask.
MemoryContexts aren't forced into using that. They're only forced into
using the 3 least significant bits for the MemoryContextMethodID.

David

Re: Non-superuser subscription owners

[Robert Haas]

On Tue, Apr 11, 2023 at 10:56 PM Amit Kapila  wrote:
> Anyway, I don't think as such there is any problem
> with restarting the worker even when the subscription owner is a
> superuser, so adjusted the check accordingly.

LGTM. I realize we could do more sophisticated things here, but I
think it's better to keep the code simple.

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

Re: Add index scan progress to pg_stat_progress_vacuum

[Imseih (AWS), Sami]

> This should be OK (also checked the code paths where the reports are
> added). Note that the patch needed a few adjustments for its
> indentation.

Thanks for the formatting corrections! This looks good to me.

--
Sami

Re: [PATCH] Use role name "system_user" instead of "user" for unsafe_tests

[Aleksander Alekseev]

Hi,

> Whether we need to have a test for this at all is perhaps a
> more interesting argument.

This was my initial thought but since somebody put it there I assumed
this is a very important test.

Any objections if we remove the tests for "user"?

-- 
Best regards,
Aleksander Alekseev

Re: v12: ERROR: subplan "InitPlan 2 (returns $4)" was not initialized

[Tom Lane]

Richard Guo  writes:
> On Wed, Apr 12, 2023 at 3:59 AM Tom Lane  wrote:
>> The v1 patch attached is enough to fix the immediate issue,
>> but there's another thing not to like, which is that we're also
>> discarding the costs associated with the initplans.  That's
>> strictly cosmetic given that all the planning decisions are
>> already made, but it still seems potentially annoying if you're
>> trying to understand EXPLAIN output.  So I'm inclined to instead
>> do something like v2 attached, which deals with that as well.
>> (On the other hand, we aren't bothering to fix up costs when
>> we move initplans around in materialize_finished_plan or
>> standard_planner ... so maybe that should be left for a patch
>> that fixes those things too.)

> +1 to the v2 patch.

Thanks for looking at this.  After sleeping on it, I'm inclined
to use the v1 patch in the back branches and do the cost fixups
only in HEAD.

> * Should we likewise set the parallel_safe flag for topmost plan in
> SS_attach_initplans?

SS_attach_initplans is assuming that costs and parallel safety
already got dealt with, either by SS_charge_for_initplans or by
equivalent processing during create_plan.  I did have an Assert
there about parallel_safe already being off in a draft version
of this patch, but dropped it after realizing that it'd have to
go away anyway when we fix things to allow parallel-safe initplans.
(I have a draft patch for that that I'll post separately.)

We could improve the comment for SS_attach_initplans to explicitly
mention parallel safety, though.  Also, I'd better go look at the
create_plan code paths to make sure they are indeed accounting
for this.

> * In standard_planner around line 443, we move any initPlans from
> top_plan to the new added Gather node.  But since we know that the
> top_plan is parallel_safe here, shouldn't it have no initPlans?

Right.  Again, I did have such an Assert there in a draft version,
then decided it wasn't useful to change temporarily.  However, the
follow-on patch removes that stanza altogether, and I suppose it
might as well remove an Assert as what's there now.  I'll make it so.

regards, tom lane

Bufmgr possible overflow

[Ranier Vilela]

Hi,

IMO I think that commit 31966b1

has an oversight.

All the logic of the changes are based on the "extend_by" variable, which
is a uint32, but in some places it is using "int", which can lead to an
overflow at some point.

I also take the opportunity to correct another oversight, regarding the
commit dad50f6

,
for possible duplicate assignment.
GetLocalBufferDescriptor was called twice.

Taking advantage of this, I promoted a scope reduction for some variables,
which I thought was opportune.

Patch attached.

regards,
Ranier Vilela


001-fix-bufmgr-extend-variable-index.patch
Description: Binary data

Re: Direct I/O

[Andrew Dunstan]

On 2023-04-12 We 01:48, Thomas Munro wrote:

On Wed, Apr 12, 2023 at 3:04 PM Thomas Munro  wrote:

On Wed, Apr 12, 2023 at 2:56 PM Christoph Berg  wrote:

I'm hitting a panic in t_004_io_direct. The build is running on
overlayfs on tmpfs/ext4 (upper/lower) which is probably a weird
combination but has worked well for building everything over the last
decade. On Debian unstable:

PANIC:  could not open file "pg_wal/00010001": Invalid argument

... I have a new idea:  perhaps it is possible to try
to open a file with O_DIRECT from perl, and if it fails like that,
skip the test.  Looking into that now.

I think I have that working OK.  Any Perl hackers want to comment on
my use of IO::File (copied from examples on the internet that showed
how to use O_DIRECT)?  I am not much of a perl hacker but according to
my package manager, IO/File.pm came with perl itself.  And the Fcntl
eval trick that I copied from File::stat, and the perl-critic
suppression that requires?



I think you can probably replace a lot of the magic here by simply saying


if (Fcntl->can("O_DIRECT")) ...


cheers


andrew

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

Re: Infinite Interval

[Ashutosh Bapat]

On Sat, Apr 8, 2023 at 8:54 PM Joseph Koshakow  wrote:
>
> I was also unable to find a definition of oscillating or monotonically
> increasing in this context. I used the existing timestamps and dates
> code to form my own definition:
>
> If there exists an two intervals with the same sign, such that adding
> them together results in an interval with a unit that is less than the
> unit of at least one of the original intervals, then that unit is
> oscillating. Otherwise it is monotonically increasing.
>
> So for example `INTERVAL '30 seconds' + INTERVAL '30 seconds'` results
> in an interval with 0 seconds, so seconds are oscillating. You couldn't
> find a similar example for days or hours, so they're monotonically
> increasing.

Thanks for the explanation with an example. Makes sense considering
that the hours and days are not convertible to their wider units
without temporal context.

>
> > SELECT t.f1 AS t, i.f1 AS i, t.f1 + i.f1 AS "add", t.f1 - i.f1 AS 
> > "subtract"
> >   FROM TIME_TBL t, INTERVAL_TBL i
> >+  WHERE isfinite(i.f1)
> >   ORDER BY 1,2;
> >
> > SELECT t.f1 AS t, i.f1 AS i, t.f1 + i.f1 AS "add", t.f1 - i.f1 AS 
> > "subtract"
> >   FROM TIMETZ_TBL t, INTERVAL_TBL i
> >+  WHERE isfinite(i.f1)
> >   ORDER BY 1,2;

Those two are operations with Time which does not allow infinity. So I
think this is fine.

> >
> > -- SQL9x OVERLAPS operator
> >@@ -287,11 +290,12 @@ SELECT f1 AS "timestamp"
> >
> > SELECT d.f1 AS "timestamp", t.f1 AS "interval", d.f1 + t.f1 AS plus
> >   FROM TEMP_TIMESTAMP d, INTERVAL_TBL t
> >+  WHERE isfinite(t.f1)
> >   ORDER BY plus, "timestamp", "interval";
> >
> > SELECT d.f1 AS "timestamp", t.f1 AS "interval", d.f1 - t.f1 AS minus
> >   FROM TEMP_TIMESTAMP d, INTERVAL_TBL t
> >-  WHERE isfinite(d.f1)
> >+  WHERE isfinite(t.f1)
> >   ORDER BY minus, "timestamp", "interval";
> I originally tried that, but the issue here is that errors propagate
> through the whole query. So if one row produces an error then no rows
> are produced and instead a single error is returned. So the rows that
> would execute, for example,
> SELECT date 'infinity' + interval '-infinity' would cause the entire
> query to error out. If you have any suggestions to get around this
> please let me know.

I modified this to WHERE isfinite(t.f1) or isfinite(d.f1). The output
contains a lot of additions with infinity::interval but that might be
ok. No errors. We could further improve it to allow operations between
infinity which do not result in error e.g, both operands being same
signed for plus and opposite signed for minus. But I think we can
leave this to the committer's judgement. Which route to choose.

>
> >With that I have reviewed the entire patch-set. Once you address these
> >comments, we can mark it as ready for committer. I already see Tom
> >looking at the patch. So that might be just a formality.
>
> Thanks so much for taking the time to review this!

My pleasure. I am very much interested to see this being part of code.
Given that the last commit fest for v16 has ended, let's target this
for v17. I will mark this as ready for committer now.

--
Best Wishes,
Ashutosh Bapat

Re: Direct I/O

[Dagfinn Ilmari Mannsåker]

Thomas Munro  writes:

> I think I have that working OK.  Any Perl hackers want to comment on
> my use of IO::File (copied from examples on the internet that showed
> how to use O_DIRECT)?  I am not much of a perl hacker but according to
> my package manager, IO/File.pm came with perl itself.

Indeed, and it has been since perl 5.003_07, released in 1996.  And Fcntl
has known about O_DIRECT since perl 5.6.0, released in 2000, so we can
safely use both.

> And the Fcntl eval trick that I copied from File::stat, and the
> perl-critic suppression that requires?
[…]
> + no strict 'refs';## no critic (ProhibitNoStrict)
> + my $val = eval { &{'Fcntl::O_DIRECT'} };
> + if (defined $val)

This trick is only needed in File::stat because it's constructing the
symbol name dynamically.  And because Fcntl by default exports all the
O_* and F_* constants it knows about, we can simply do:

if (defined &O_DIRECT)
> + {
> + use Fcntl qw(O_DIRECT);

The `use Fcntl;` above will already have imported this, so this is
redundant.

- ilmari

Re: Direct I/O

[Dagfinn Ilmari Mannsåker]

Andrew Dunstan  writes:

> On 2023-04-12 We 01:48, Thomas Munro wrote:
>> On Wed, Apr 12, 2023 at 3:04 PM Thomas Munro  wrote:
>>> On Wed, Apr 12, 2023 at 2:56 PM Christoph Berg  wrote:
 I'm hitting a panic in t_004_io_direct. The build is running on
 overlayfs on tmpfs/ext4 (upper/lower) which is probably a weird
 combination but has worked well for building everything over the last
 decade. On Debian unstable:

 PANIC:  could not open file "pg_wal/00010001": Invalid 
 argument
>>> ... I have a new idea:  perhaps it is possible to try
>>> to open a file with O_DIRECT from perl, and if it fails like that,
>>> skip the test.  Looking into that now.
>> I think I have that working OK.  Any Perl hackers want to comment on
>> my use of IO::File (copied from examples on the internet that showed
>> how to use O_DIRECT)?  I am not much of a perl hacker but according to
>> my package manager, IO/File.pm came with perl itself.  And the Fcntl
>> eval trick that I copied from File::stat, and the perl-critic
>> suppression that requires?
>
>
> I think you can probably replace a lot of the magic here by simply saying
>
>
> if (Fcntl->can("O_DIRECT")) ...

Fcntl->can() is true for all constants that Fcntl knows about, whether
or not they are defined for your OS. `defined &O_DIRECT` is the simplest
check, see my other reply to Thomas.

> cheers
>
>
> andrew

- ilmari

Re: Fix incorrect start up costs for WindowAgg paths (bug #17862)

[Andy Fan]

On Wed, Apr 12, 2023 at 5:04 PM David Rowley  wrote:

>
> With the attached patch, that turns into:
>

The concept of startup_tuples for a WindowAgg looks good to me, but I
can't follow up with the below line:

+ return clamp_row_est(partition_tuples * DEFAULT_INEQ_SEL);

# select count(*) over() from tenk1 limit 1;
 count
---
 1  -->  We need to scan all the tuples.

Should we just return clamp_row_est(partition_tuples)?


-- 
Best Regards
Andy Fan

Re: longfin missing gssapi_ext.h

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > Understood.  Please find attached the updated patch with changes to the
> > commit message to indicate that we now require MIT Kerberos, an
> > additional explicit check for gssapi_ext.h in configure.ac/configure,
> > along with updated documentation explicitly saying we require MIT
> > Kerberos for GSSAPI support.
> 
> Um ... could you package this as a straight un-revert of the
> previous commit, then a delta patch?  Would be easier to review.

Sure, reworked that way and attached.

Thanks,

Stephen
From 9acb2ef2e6a35544e28d690978ba8d5e8c062f7e Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 12 Apr 2023 09:33:39 -0400
Subject: [PATCH 1/2] Revert "Revert "Add support for Kerberos credential
 delegation""

This reverts commit 3d03b24c3 (Add support for Kerberos credential
delegation) which was reverted on the grounds of concern about
portability, but on further review and discussion, it's clear that
we are better off simply explicitly requiring MIT Kerberos as that seems
to be the only GSSAPI library currently that's under proper maintenance
and ongoing development.  The API used for storing credentials was added
to MIT Kerberos over a decade ago while for the other libraries which
appear to be mainly based on Heimdal, which exists explicitly to be a
re-implementation of MIT Kerberos, the API never made it to a released
version (even though it was added to the Heimdal git repo over 5 years
ago..).

This post-feature-freeze change was approved by the RMT.

Discussion: https://postgr.es/m/ZDDO6jaESKaBgej0%40tamriel.snowman.net
---
 contrib/dblink/dblink.c   | 127 ---
 contrib/dblink/expected/dblink.out|   4 +-
 contrib/postgres_fdw/connection.c |  72 +++-
 .../postgres_fdw/expected/postgres_fdw.out|  19 +-
 contrib/postgres_fdw/option.c |   6 +
 contrib/postgres_fdw/sql/postgres_fdw.sql |   3 +-
 doc/src/sgml/config.sgml  |  17 +
 doc/src/sgml/dblink.sgml  |   5 +-
 doc/src/sgml/libpq.sgml   |  41 +++
 doc/src/sgml/monitoring.sgml  |   9 +
 doc/src/sgml/postgres-fdw.sgml|   7 +-
 src/backend/catalog/system_views.sql  |   3 +-
 src/backend/foreign/foreign.c |   1 +
 src/backend/libpq/auth.c  |  13 +-
 src/backend/libpq/be-gssapi-common.c  |  53 +++
 src/backend/libpq/be-secure-gssapi.c  |  26 +-
 src/backend/utils/activity/backend_status.c   |   1 +
 src/backend/utils/adt/pgstatfuncs.c   |  21 +-
 src/backend/utils/init/postinit.c |   8 +-
 src/backend/utils/misc/guc_tables.c   |  10 +
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/include/catalog/pg_proc.dat   |   6 +-
 src/include/libpq/auth.h  |   1 +
 src/include/libpq/be-gssapi-common.h  |   3 +
 src/include/libpq/libpq-be.h  |   2 +
 src/include/utils/backend_status.h|   1 +
 src/interfaces/libpq/exports.txt  |   1 +
 src/interfaces/libpq/fe-auth.c|  15 +-
 src/interfaces/libpq/fe-connect.c |  17 +
 src/interfaces/libpq/fe-secure-gssapi.c   |  23 +-
 src/interfaces/libpq/libpq-fe.h   |   1 +
 src/interfaces/libpq/libpq-int.h  |   2 +
 src/test/kerberos/Makefile|   3 +
 src/test/kerberos/t/001_auth.pl   | 331 --
 src/test/perl/PostgreSQL/Test/Utils.pm|  27 ++
 src/test/regress/expected/rules.out   |  11 +-
 36 files changed, 755 insertions(+), 136 deletions(-)

diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 78a8bcee6e..55f75eff36 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -48,6 +48,7 @@
 #include "funcapi.h"
 #include "lib/stringinfo.h"
 #include "libpq-fe.h"
+#include "libpq/libpq-be.h"
 #include "libpq/libpq-be-fe-helpers.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
@@ -111,7 +112,8 @@ static HeapTuple get_tuple_of_interest(Relation rel, int *pkattnums, int pknumat
 static Relation get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclMode aclmode);
 static char *generate_relation_name(Relation rel);
 static void dblink_connstr_check(const char *connstr);
-static void dblink_security_check(PGconn *conn, remoteConn *rconn);
+static bool dblink_connstr_has_pw(const char *connstr);
+static void dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr);
 static void dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
 			 bool fail, const char *fmt,...) pg_attribute_printf(5, 6);
 static char *get_connect_string(const char *servername);
@@ -213,7 +215,7 @@ dblink_get_conn(char *conname_or_str,
 	 errmsg("could not establish connection"),
 	 errdetail_internal("%s", msg)));
 		}
-		dblink_security_check(c

Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply

[Fujii Masao]

On 2023/04/12 12:00, Kyotaro Horiguchi wrote:

At Wed, 12 Apr 2023 03:36:01 +0900, Fujii Masao  
wrote in

Attached patch fixes this issue. It ensures that postgres_fdw only
waits
for a reply if a cancel request is actually issued. Additionally,
it improves PQgetCancel() to set error messages in certain error
cases,
such as when out of memory occurs and malloc() fails. Moreover,
it enhances postgres_fdw to report a warning message when
PQgetCancel()
returns NULL, explaining the reason for the NULL value.

Thought?


I wondered why the connection didn't fail in the first place. After
digging into it, I found (or remembered) that local (or AF_UNIX)
connections ignore the timeout value at making a connection. I think


BTW, you can reproduce the issue even when using a TCP connection
instead of a Unix domain socket by specifying a very large number
in the "keepalives" connection parameter for the foreign server.
Here is an example:

-
create server loopback foreign data wrapper postgres_fdw options (host 
'127.0.0.1', port '5432', keepalives '999');
-

The reason behind this issue is that PQconnectPoll() parses
the "keepalives" parameter value by simply using strtol(),
while PQgetCancel() uses parse_int_param(). To fix this issue,
it might be better to update PQconnectPoll() so that it uses
parse_int_param() for parsing the "keepalives" parameter.




the real issue here is that PGgetCancel is unnecessarily checking its
value and failing as a result. Otherwise there would be no room for
failure in the call to PQgetCancel() at that point in the example
case.

PQconnectPoll should remove the ignored parameters at connection or
PQgetCancel should ingore the unreferenced (or unchecked)
parameters. For example, the below diff takes the latter way and seems
working (for at least AF_UNIX connections)


To clarify, are you suggesting that PQgetCancel() should
only parse the parameters for TCP connections
if cancel->raddr.addr.ss_family != AF_UNIX?
If so, I think that's a good idea.



Of course, it's not great that pgfdw_cancel_query_begin() ignores the
result from PQgetCancel(), but I think we don't need another ereport.


Can you please clarify why you suggest avoiding outputting
the warning message when PQgetCancel() returns NULL?
I think it is important to inform the user when an error
occurs and a cancel request cannot be sent, as this information
can help them identify the cause of the problem (such as
setting an overly large value for the keepalives parameter).

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Note new NULLS NOT DISTINCT on unique index tutorial page

[David Gilman]

The SQL Language part of the docs has a brief page on unique indexes.
It doesn't mention the new NULLS NOT DISTINCT functionality on unique
indexes and this is a good place to mention it, as it already warns
the user about the old/default behavior.

-- 
David Gilman
:DG<
From 5cb84716462611b84e6b2fbdb35172ec43b52db8 Mon Sep 17 00:00:00 2001
From: David Gilman 
Date: Wed, 12 Apr 2023 10:28:09 -0400
Subject: [PATCH] Note NULLS NOT DISTINCT on unique index tutorial

---
 doc/src/sgml/indices.sgml | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/indices.sgml b/doc/src/sgml/indices.sgml
index 0c3fcfd62f8e..0a67e66f5065 100644
--- a/doc/src/sgml/indices.sgml
+++ b/doc/src/sgml/indices.sgml
@@ -671,9 +671,11 @@ CREATE UNIQUE INDEX name ON 
table
When an index is declared unique, multiple table rows with equal
-   indexed values are not allowed.  Null values are not considered
-   equal.  A multicolumn unique index will only reject cases where all
-   indexed columns are equal in multiple rows.
+   indexed values are not allowed.  By default, null values in a unique column 
+   are not considered equal, allowing multiple nulls in the column.
+   Use NULLS NOT DISTINCT to treat column nulls as equal, 
+   allowing only a single null value in an indexed column. A multicolumn 
unique index 
+   will only reject cases where all indexed columns are equal in multiple rows.
   
 
   

Re: longfin missing gssapi_ext.h

[Jonathan S. Katz]

On 4/12/23 10:33 AM, Stephen Frost wrote:

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:

Stephen Frost  writes:

Understood.  Please find attached the updated patch with changes to the
commit message to indicate that we now require MIT Kerberos, an
additional explicit check for gssapi_ext.h in configure.ac/configure,
along with updated documentation explicitly saying we require MIT
Kerberos for GSSAPI support.


Um ... could you package this as a straight un-revert of the
previous commit, then a delta patch?  Would be easier to review.


Sure, reworked that way and attached.


Docs read well. A few questions/commenets:

* On [1] -- do we want to add a note that it's not just Kerberos, but 
MIT Kerberos?


* On [2] -- we mention "kadmin tool of MIT-compatible Kerberos 5" which 
is AIUI is still technically correct, but do we want to drop the 
"-compatible?" (precedent in [3])


Thanks,

Jonathan

[1] https://www.postgresql.org/docs/devel/install-requirements.html
[2] https://www.postgresql.org/docs/devel/gssapi-auth.html
[3] https://www.postgresql.org/docs/devel/regress-run.html


OpenPGP_signature
Description: OpenPGP digital signature

Re: longfin missing gssapi_ext.h

[Daniel Gustafsson]

> On 12 Apr 2023, at 16:33, Stephen Frost  wrote:

> Sure, reworked that way and attached.

While not changed in this hunk, does the comment regarding Heimdal still apply?

@@ -918,6 +919,7 @@ pg_GSS_recvauth(Port *port)
int mtype;
StringInfoData buf;
gss_buffer_desc gbuf;
+   gss_cred_id_t delegated_creds;
 
/*
 * Use the configured keytab, if there is one.  Unfortunately, Heimdal

--
Daniel Gustafsson

Re: longfin missing gssapi_ext.h

[Stephen Frost]

Greetings,

* Jonathan S. Katz (jk...@postgresql.org) wrote:
> On 4/12/23 10:33 AM, Stephen Frost wrote:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > > Stephen Frost  writes:
> > > > Understood.  Please find attached the updated patch with changes to the
> > > > commit message to indicate that we now require MIT Kerberos, an
> > > > additional explicit check for gssapi_ext.h in configure.ac/configure,
> > > > along with updated documentation explicitly saying we require MIT
> > > > Kerberos for GSSAPI support.
> > > 
> > > Um ... could you package this as a straight un-revert of the
> > > previous commit, then a delta patch?  Would be easier to review.
> > 
> > Sure, reworked that way and attached.
> 
> Docs read well. A few questions/commenets:
> 
> * On [1] -- do we want to add a note that it's not just Kerberos, but MIT
> Kerberos?

Yes, makes sense, updated.

> * On [2] -- we mention "kadmin tool of MIT-compatible Kerberos 5" which is
> AIUI is still technically correct, but do we want to drop the "-compatible?"
> (precedent in [3])

Yup, cleaned that up also.

Updated patch set attached.

Thanks!

Stephen
From 9acb2ef2e6a35544e28d690978ba8d5e8c062f7e Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 12 Apr 2023 09:33:39 -0400
Subject: [PATCH 1/2] Revert "Revert "Add support for Kerberos credential
 delegation""

This reverts commit 3d03b24c3 (Add support for Kerberos credential
delegation) which was reverted on the grounds of concern about
portability, but on further review and discussion, it's clear that
we are better off simply explicitly requiring MIT Kerberos as that seems
to be the only GSSAPI library currently that's under proper maintenance
and ongoing development.  The API used for storing credentials was added
to MIT Kerberos over a decade ago while for the other libraries which
appear to be mainly based on Heimdal, which exists explicitly to be a
re-implementation of MIT Kerberos, the API never made it to a released
version (even though it was added to the Heimdal git repo over 5 years
ago..).

This post-feature-freeze change was approved by the RMT.

Discussion: https://postgr.es/m/ZDDO6jaESKaBgej0%40tamriel.snowman.net
---
 contrib/dblink/dblink.c   | 127 ---
 contrib/dblink/expected/dblink.out|   4 +-
 contrib/postgres_fdw/connection.c |  72 +++-
 .../postgres_fdw/expected/postgres_fdw.out|  19 +-
 contrib/postgres_fdw/option.c |   6 +
 contrib/postgres_fdw/sql/postgres_fdw.sql |   3 +-
 doc/src/sgml/config.sgml  |  17 +
 doc/src/sgml/dblink.sgml  |   5 +-
 doc/src/sgml/libpq.sgml   |  41 +++
 doc/src/sgml/monitoring.sgml  |   9 +
 doc/src/sgml/postgres-fdw.sgml|   7 +-
 src/backend/catalog/system_views.sql  |   3 +-
 src/backend/foreign/foreign.c |   1 +
 src/backend/libpq/auth.c  |  13 +-
 src/backend/libpq/be-gssapi-common.c  |  53 +++
 src/backend/libpq/be-secure-gssapi.c  |  26 +-
 src/backend/utils/activity/backend_status.c   |   1 +
 src/backend/utils/adt/pgstatfuncs.c   |  21 +-
 src/backend/utils/init/postinit.c |   8 +-
 src/backend/utils/misc/guc_tables.c   |  10 +
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/include/catalog/pg_proc.dat   |   6 +-
 src/include/libpq/auth.h  |   1 +
 src/include/libpq/be-gssapi-common.h  |   3 +
 src/include/libpq/libpq-be.h  |   2 +
 src/include/utils/backend_status.h|   1 +
 src/interfaces/libpq/exports.txt  |   1 +
 src/interfaces/libpq/fe-auth.c|  15 +-
 src/interfaces/libpq/fe-connect.c |  17 +
 src/interfaces/libpq/fe-secure-gssapi.c   |  23 +-
 src/interfaces/libpq/libpq-fe.h   |   1 +
 src/interfaces/libpq/libpq-int.h  |   2 +
 src/test/kerberos/Makefile|   3 +
 src/test/kerberos/t/001_auth.pl   | 331 --
 src/test/perl/PostgreSQL/Test/Utils.pm|  27 ++
 src/test/regress/expected/rules.out   |  11 +-
 36 files changed, 755 insertions(+), 136 deletions(-)

diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 78a8bcee6e..55f75eff36 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -48,6 +48,7 @@
 #include "funcapi.h"
 #include "lib/stringinfo.h"
 #include "libpq-fe.h"
+#include "libpq/libpq-be.h"
 #include "libpq/libpq-be-fe-helpers.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
@@ -111,7 +112,8 @@ static HeapTuple get_tuple_of_interest(Relation rel, int *pkattnums, int pknumat
 static Relation get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclMode aclmode);
 static char *generate_relation_name(Relation rel);
 static void dblink_connstr_check(const char *connstr);
-static void dblink_security_check(PGconn *conn, remoteCo

Re: longfin missing gssapi_ext.h

[Jonathan S. Katz]

On 4/12/23 10:47 AM, Stephen Frost wrote:

Greetings,

* Jonathan S. Katz (jk...@postgresql.org) wrote:

On 4/12/23 10:33 AM, Stephen Frost wrote:

* Tom Lane (t...@sss.pgh.pa.us) wrote:

Stephen Frost  writes:

Understood.  Please find attached the updated patch with changes to the
commit message to indicate that we now require MIT Kerberos, an
additional explicit check for gssapi_ext.h in configure.ac/configure,
along with updated documentation explicitly saying we require MIT
Kerberos for GSSAPI support.


Um ... could you package this as a straight un-revert of the
previous commit, then a delta patch?  Would be easier to review.


Sure, reworked that way and attached.


Docs read well. A few questions/commenets:

* On [1] -- do we want to add a note that it's not just Kerberos, but MIT
Kerberos?


Yes, makes sense, updated.


* On [2] -- we mention "kadmin tool of MIT-compatible Kerberos 5" which is
AIUI is still technically correct, but do we want to drop the "-compatible?"
(precedent in [3])


Yup, cleaned that up also.

Updated patch set attached.


Thanks! I'll sign off on the docs portion.

The meson build code looks good to me (I just compared it to what 
already exists). Similar comment to the autoconf code.


Thanks,

Jonathan


OpenPGP_signature
Description: OpenPGP digital signature

Re: longfin missing gssapi_ext.h

[Stephen Frost]

Greetings,

* Daniel Gustafsson (dan...@yesql.se) wrote:
> > On 12 Apr 2023, at 16:33, Stephen Frost  wrote:
> > Sure, reworked that way and attached.
> 
> While not changed in this hunk, does the comment regarding Heimdal still 
> apply?
> 
> @@ -918,6 +919,7 @@ pg_GSS_recvauth(Port *port)
>   int mtype;
>   StringInfoData buf;
>   gss_buffer_desc gbuf;
> + gss_cred_id_t delegated_creds;
>  
>   /*
>* Use the configured keytab, if there is one.  Unfortunately, Heimdal

Good catch.  No, it doesn't.  I'm not anxious to actually change that
code at this point but we could certainly consider changing it in the
future.  I'll update this comment (and the identical one in
secure_open_gssapi) accordingly.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Wrong results from Parallel Hash Full Join

[Melanie Plageman]

On Wed, Apr 12, 2023 at 7:36 AM Richard Guo  wrote:
>
> I came across $subject and reduced the repro query as below.
>
> create table a (i int);
> create table b (i int);
> insert into a values (1);
> insert into b values (2);
> update b set i = 2;
>
> set min_parallel_table_scan_size to 0;
> set parallel_tuple_cost to 0;
> set parallel_setup_cost to 0;
>
> # explain (costs off) select * from a full join b on a.i = b.i;
> QUERY PLAN
> --
>  Gather
>Workers Planned: 2
>->  Parallel Hash Full Join
>  Hash Cond: (a.i = b.i)
>  ->  Parallel Seq Scan on a
>  ->  Parallel Hash
>->  Parallel Seq Scan on b
> (7 rows)
>
> # select * from a full join b on a.i = b.i;
>  i | i
> ---+---
>  1 |
> (1 row)
>
> Tuple (NULL, 2) is missing from the results.

Thanks so much for reporting this, Richard. This is a fantastic minimal
repro!

So, I looked into this, and it seems that, as you can imagine, the tuple
in b is hot updated, resulting in a heap only tuple.

 t_ctid |  raw_flags
+--
 (0,2)  | {HEAP_XMIN_COMMITTED,HEAP_XMAX_COMMITTED,HEAP_HOT_UPDATED}
 (0,2)  | {HEAP_XMIN_COMMITTED,HEAP_XMAX_INVALID,HEAP_UPDATED,HEAP_ONLY_TUPLE}

In ExecParallelScanHashTableForUnmatched() we don't emit the
NULL-extended tuple because HeapTupleHeaderHasMatch() is true for our
desired tuple.

while (hashTuple != NULL)
{
if (!HeapTupleHeaderHasMatch(HJTUPLE_MINTUPLE(hashTuple)))
{

HeapTupleHeaderHasMatch() checks if HEAP_TUPLE_HAS_MATCH is set.

In htup_details.h, you will see that HEAP_TUPLE_HAS_MATCH is defined as
HEAP_ONLY_TUPLE
/*
 * HEAP_TUPLE_HAS_MATCH is a temporary flag used during hash joins.  It is
 * only used in tuples that are in the hash table, and those don't need
 * any visibility information, so we can overlay it on a visibility flag
 * instead of using up a dedicated bit.
 */
#define HEAP_TUPLE_HAS_MATCHHEAP_ONLY_TUPLE /* tuple has a join match */

If you redefine HEAP_TUPLE_HAS_MATCH as something that isn't already
used, say 0x1800, the query returns correct results.

QUERY PLAN
--
 Gather
   Workers Planned: 2
   ->  Parallel Hash Full Join
 Hash Cond: (a.i = b.i)
 ->  Parallel Seq Scan on a
 ->  Parallel Hash
   ->  Parallel Seq Scan on b
(7 rows)

 i | i
---+---
 1 |
   | 2
(2 rows)

The question is, why does this only happen for a parallel full hash join?

unpa
postgres=# explain (costs off) select * from a full join b on a.i = b.i;
QUERY PLAN
---
 Hash Full Join
   Hash Cond: (a.i = b.i)
   ->  Seq Scan on a
   ->  Hash
 ->  Seq Scan on b
(5 rows)

postgres=#  select * from a full join b on a.i = b.i;
 i | i
---+---
 1 |
   | 2
(2 rows)

I imagine it has something to do with what tuples are put in the
parallel hashtable. I am about to investigate that but just wanted to
share what I had so far.

- Melanie

Re: refactoring basebackup.c

[Justin Pryzby]

On Fri, Mar 24, 2023 at 10:46:37AM -0400, Robert Haas wrote:
> On Thu, Mar 23, 2023 at 4:11 PM Robert Haas  wrote:
> > On Wed, Mar 22, 2023 at 10:09 PM Thomas Munro  
> > wrote:
> > > > BaseBackupSync is not documented
> > > > BaseBackupWrite is not documented
> > >
> > > [Resending with trimmed CC: list, because the mailing list told me to
> > > due to a blocked account, sorry if you already got the above.]
> >
> > Bummer. I'll write a patch to fix that tomorrow, unless somebody beats me 
> > to it.
> 
> Here's a patch for that, and a patch to add the missing error check
> Peter noticed.

I think these maybe got forgotten ?

Re: Improve the performance of nested loop join in the case of partitioned inner table

[Alexandr Nikulin]

The following tests demonstrate the speedup which may be achieved with my
patch:

1. Add to postgresql.conf
enable_hashjoin = OFF
enable_mergejoin = OFF
enable_material = OFF
enable_bitmapscan = OFF
enable_nestloop = ON
max_parallel_workers_per_gather = 0
enable_memoize = OFF

2. create test tables:

create table test_part(id int primary key) partition by range(id);
create table test_part0 partition of test_part for values from (0) to
(100);
create table test_part1 partition of test_part for values from (100) to
(200);
insert into test_part select id from generate_series(0, 100-1) as id;

create table ids(id int, name varchar); create index on ids(ascii(name));
insert into ids select id, 'worst case' as name from generate_series(0,
100-1) as id;
insert into ids select 123456, 'best case' as name from generate_series(0,
100-1) as id;

3. run tests:

explain analyze select * from ids join test_part on ids.id=test_part.id
where ascii(ids.name)=ascii('best case');
explain analyze select * from ids join test_part on ids.id=test_part.id
where ascii(ids.name)=ascii('worst case');

The average results on my machine are as follows:

 | vanila postgres | patched postgres
best case| 2286 ms | 1924 ms
worst case   | 2278 ms | 2360 ms

So far I haven't refactored the patch as per David's advice. I just want to
understand if we need such an optimization?


чт, 23 мар. 2023 г. в 17:05, David Rowley :

> On Thu, 23 Mar 2023 at 19:46, Alexandr Nikulin
>  wrote:
> > I propose to slightly improve the performance of nested loop join in the
> case of partitioned inner table.
> > As I see in the code, the backend looks for the partition of the inner
> table each time after fetch a new row from the outer table.
> > These searches can take a significant amount of time.
> > But we can skip this step if the nested loop parameter(s) was(re) not
> changed since the previous row fetched from the outer table
>
> I think if we were to do something like that, then it should be done
> in nodeAppend.c and nodeMergeAppend.c. That way you can limit it to
> only checking parameters that partition pruning needs to care about.
> That does mean you'd need to find somewhere to cache the parameter
> values, however. Doing it in nodeNestloop.c means you're doing it when
> the inner subplan is something that does not suffer from the
> additional overhead you want to avoid, e.g an Index Scan.
>
> Also, generally, if you want to get anywhere with a performance patch,
> you should post performance results from before and after your change.
> Also include your benchmark setup and relevant settings for how you
> got those results.  For this case, you'll want a best case (parameter
> value stays the same) and a worst case, where the parameter value
> changes on each outer row.  I expect you're going to add overhead to
> this case as your additional checks will always detect the parameter
> has changed as that'll always require partition pruning to be executed
> again. We'll want to know if that overhead is high enough for us not
> to want to do this.
>
> I'll be interested to see a test that as some varlena parameter of say
> a few hundred bytes to see how much overhead testing if that parameter
> has changed when the pruning is being done on a HASH partitioned
> table.  HASH partitioning should prune quite a bit faster than both
> LIST and RANGE as the hashing is effectively O(1) vs O(log2 N) (N
> being the number of Datums in the partition bounds). I'd expect a
> measurable additional overhead with the patch when the parameter
> changes on each outer row.
>
> David
>

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Robert Haas]

On Tue, Apr 11, 2023 at 10:17 PM Yurii Rashkovskii  wrote:
> I appreciate your support on the pid file concern. What questions do you have 
> about this feature with regard to its desirability and/or implementation? I'd 
> love to learn from your insight and address any of those if I can.

I don't have any particularly specific concerns. But, you know, if a
bunch of other people, especially people already known the community
showed up on this thread to say "hey, I'd like that too" or "that
would be better than what we have now," well then that would make me
think "hey, we should probably move forward with this thing." But so
far the only people to comment are Tom and Andrew. Tom, in addition to
complaining about the PID file thing, also basically said that the
feature didn't seem necessary to him, and Andrew's comments seem to me
to suggest the same thing. So it kind of seems like you've convinced
zero people that this is a thing we should have, and that's not very
many.

It happens from time to time on this mailing list that somebody shows
up to propose a feature where I say to myself "hmm, that doesn't sound
like an intrinsically terrible idea, but it sounds like it might be
specific enough that only the person proposing it would ever use it."
For instance, someone might propose a new backslash command for psql
that runs an SQL query that produces some output which that person
finds useful. There is no big design problem there, but psql is
already pretty cluttered with commands that look like line noise, so
we shouldn't add a new one on the strength of one person wanting it.
Each feature, even if it's minor, has some cost. New releases need to
keep it working, which may mean that it needs a test, and then the
test is another thing that you have to keep working, and it also takes
time to run every time anyone does make check-world. These aren't big
costs and don't set a high bar for adding new features, but they do
mean, at least IMHO, that one person wanting a feature that isn't
obviously of general utility is not good enough. I think all of that
also applies to this feature.

I haven't reviewed the code in detail. It at least has some style
issues, but worrying about that seems premature.

I mostly just wanted to say that I disagreed with Tom about the
particular point on postmaster.pid, without really expressing an
opinion about anything else.

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

Re: is_superuser is not documented

[Fujii Masao]

On 2023/04/12 5:41, Joseph Koshakow wrote:

Having said all that I actually think this is the best place for
is_superuser since it doesn't seem to fit in anywhere else.


Yeah, I also could not find more appropriate place for is_superuser than there.



I was implying that I thought it would have made more sense for
is_superuser to be implemented as a function, behave as a function,
and not be visible via SHOW. However, there may have been a good reason
not to do this and it may already be too late for that.


The is_superuser parameter is currently marked as GUC_REPORT and
its value is automatically reported to a client. If we change
it to a function, we will need to add functionality to automatically
report the return value of the function to a client, which could
be overkill.



In my opinion, this is ready to be committed.


Thanks! Given that we have already exceeded the feature freeze date,
I'm thinking to commit this change at the next CommitFest.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: meson documentation build open issues

[Peter Eisentraut]

On 07.04.23 16:39, Andrew Dunstan wrote:

5. There doesn't appear to be an equivalent of "make world" and "make
install-world" that includes documentation builds.


This has been addressed with the additional meson auto options.  But it
seems that these options only control building, not installing, so 
there is

no "install-world" aspect yet.


I'm not following - install-world install docs if the docs feature is
available, and not if not?


I had expected that if meson setup enables the 'docs' feature, then 
meson compile will build the documentation, which happens, and meson 
install will install it, which does not happen.


"meson compile" doesn't seem to build the docs by default ( see 
), and I'd rather it didn't, building the docs is a separate and optional step for the buildfarm.


You can control this with the "docs" option for meson, as of recently.

Re: longfin missing gssapi_ext.h

[Tom Lane]

Stephen Frost  writes:
> Updated patch set attached.

LGTM

regards, tom lane

Re: refactoring basebackup.c

[Robert Haas]

On Wed, Apr 12, 2023 at 10:57 AM Justin Pryzby  wrote:
> I think these maybe got forgotten ?

Committed.

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

Re: Direct I/O

[Andrew Dunstan]

On 2023-04-12 We 10:23, Dagfinn Ilmari Mannsåker wrote:

Andrew Dunstan  writes:


On 2023-04-12 We 01:48, Thomas Munro wrote:

On Wed, Apr 12, 2023 at 3:04 PM Thomas Munro   wrote:

On Wed, Apr 12, 2023 at 2:56 PM Christoph Berg   wrote:

I'm hitting a panic in t_004_io_direct. The build is running on
overlayfs on tmpfs/ext4 (upper/lower) which is probably a weird
combination but has worked well for building everything over the last
decade. On Debian unstable:

PANIC:  could not open file "pg_wal/00010001": Invalid argument

... I have a new idea:  perhaps it is possible to try
to open a file with O_DIRECT from perl, and if it fails like that,
skip the test.  Looking into that now.

I think I have that working OK.  Any Perl hackers want to comment on
my use of IO::File (copied from examples on the internet that showed
how to use O_DIRECT)?  I am not much of a perl hacker but according to
my package manager, IO/File.pm came with perl itself.  And the Fcntl
eval trick that I copied from File::stat, and the perl-critic
suppression that requires?


I think you can probably replace a lot of the magic here by simply saying


if (Fcntl->can("O_DIRECT")) ...

Fcntl->can() is true for all constants that Fcntl knows about, whether
or not they are defined for your OS. `defined &O_DIRECT` is the simplest
check, see my other reply to Thomas.




My understanding was that Fcntl only exported constants known to the OS. 
That's certainly what its docco suggests, e.g.:


    By default your system's F_* and O_* constants (eg, F_DUPFD and 
O_CREAT)

    and the FD_CLOEXEC constant are exported into your namespace.


cheers


andrew




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

Add two missing tests in 035_standby_logical_decoding.pl

[Drouvot, Bertrand]

hi hackers,

In the logical decoding on standby thread [1], Andres proposed 2 new tests 
(that I did
not find the time to complete before the finish line):

- Test that we can subscribe to the standby (with the publication created on 
the primary)
- Verify that invalidated logical slots do not lead to retaining WAL

Please find those 2 missing tests in the patch proposal attached.

A few words about them:

1) Regarding the subscription test:

It modifies wait_for_catchup() to take into account the case where the 
requesting
node is in recovery mode. Indeed, without that change, 
wait_for_subscription_sync() was
failing with:

"
error running SQL: 'psql::1: ERROR:  recovery is in progress
HINT:  WAL control functions cannot be executed during recovery.'
while running 'psql -XAtq -d port=61441 host=/tmp/45dt3wqs2p dbname='postgres' 
-f - -v ON_ERROR_STOP=1' with sql 'SELECT pg_current_wal_lsn()'
"

2) Regarding the WAL file not retained test:

As it's not possible to execute pg_switch_wal() and friends on a standby, this 
is
done on the primary. Also checking that the WAL file (linked to a restart_lsn 
of an invalidate
slot) has been removed is done directly at the os/directory level.

The attached patch also removes:

"
-log_min_messages = 'debug2'
-log_error_verbosity = verbose
"

as also discussed in [1].

I'm not sure if adding those 2 tests should be considered as an open item. I 
can add this open item
if we think that makes sense. I'd be happy to do so but it looks like I don't 
have the privileges
to edit https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items

Regards,

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

[1]: 
https://www.postgresql.org/message-id/6d801661-e21b-7326-be1b-f90d904da66a%40gmail.comdiff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm 
b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 6f7f4e5de4..819667d42a 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2644,7 +2644,16 @@ sub wait_for_catchup
}
if (!defined($target_lsn))
{
-   $target_lsn = $self->lsn('write');
+   my $isrecovery = $self->safe_psql('postgres', "SELECT 
pg_is_in_recovery()");
+   chomp($isrecovery);
+   if ($isrecovery eq 't')
+   {
+   $target_lsn = $self->lsn('replay');
+   }
+   else
+   {
+   $target_lsn = $self->lsn('write');
+   }
}
print "Waiting for replication conn "
  . $standby_name . "'s "
diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl 
b/src/test/recovery/t/035_standby_logical_decoding.pl
index 94a8384c31..09bc417356 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -9,13 +9,19 @@ use warnings;
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
+use Time::HiRes qw(usleep);
 
-my ($stdin, $stdout, $stderr, $cascading_stdout, $cascading_stderr, $ret, 
$handle, $slot);
+my ($stdin, $stdout,$stderr,
+   $cascading_stdout,  $cascading_stderr,  $subscriber_stdin,
+   $subscriber_stdout, $subscriber_stderr, $ret,
+   $handle,$slot);
 
 my $node_primary = PostgreSQL::Test::Cluster->new('primary');
 my $node_standby = PostgreSQL::Test::Cluster->new('standby');
 my $node_cascading_standby = 
PostgreSQL::Test::Cluster->new('cascading_standby');
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
 my $default_timeout = $PostgreSQL::Test::Utils::timeout_default;
+my $psql_timeout= IPC::Run::timer(2 * $default_timeout);
 my $res;
 
 # Name for the physical slot on primary
@@ -235,8 +241,6 @@ $node_primary->append_conf('postgresql.conf', q{
 wal_level = 'logical'
 max_replication_slots = 4
 max_wal_senders = 4
-log_min_messages = 'debug2'
-log_error_verbosity = verbose
 });
 $node_primary->dump_info;
 $node_primary->start;
@@ -269,7 +273,8 @@ $node_standby->init_from_backup(
has_streaming => 1,
has_restoring => 1);
 $node_standby->append_conf('postgresql.conf',
-   qq[primary_slot_name = '$primary_slotname']);
+   qq[primary_slot_name = '$primary_slotname'
+   max_replication_slots = 5]);
 $node_standby->start;
 $node_primary->wait_for_replay_catchup($node_standby);
 $node_standby->safe_psql('testdb', qq[SELECT * FROM 
pg_create_physical_replication_slot('$standby_physical_slotname');]);
@@ -287,6 +292,27 @@ $node_cascading_standby->append_conf('postgresql.conf',
 $node_cascading_standby->start;
 $node_standby->wait_for_replay_catchup($node_cascading_standby, $node_primary);
 
+###
+# Initialize subscriber node
+###
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->append_conf('postgresql.conf', 'max_replicati

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Andrew Dunstan]

On 2023-04-12 We 11:01, Robert Haas wrote:

On Tue, Apr 11, 2023 at 10:17 PM Yurii Rashkovskii  wrote:

I appreciate your support on the pid file concern. What questions do you have 
about this feature with regard to its desirability and/or implementation? I'd 
love to learn from your insight and address any of those if I can.

I don't have any particularly specific concerns. But, you know, if a
bunch of other people, especially people already known the community
showed up on this thread to say "hey, I'd like that too" or "that
would be better than what we have now," well then that would make me
think "hey, we should probably move forward with this thing." But so
far the only people to comment are Tom and Andrew. Tom, in addition to
complaining about the PID file thing, also basically said that the
feature didn't seem necessary to him, and Andrew's comments seem to me
to suggest the same thing. So it kind of seems like you've convinced
zero people that this is a thing we should have, and that's not very
many.



Not quite, I just suggested looking at a different approach.  I'm not 
opposed to the idea in principle.


I agree with you that parsing the pid file shouldn't be hard or 
unreasonable.



cheers


andrew


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

Re: longfin missing gssapi_ext.h

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > Updated patch set attached.
> 
> LGTM

Great, thanks.

I cleaned up the commit messages a bit more and added links to the
discussion.  If there isn't anything more then I'll plan to push these
later today or tomorrow.

Thanks again!

Stephen
From a73e19808c8cb0e5075e6c86ba0d8d29538b1eeb Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Wed, 12 Apr 2023 09:33:39 -0400
Subject: [PATCH 1/2] De-Revert "Add support for Kerberos credential
 delegation"

This reverts commit 3d03b24c3 (Revert Add support for Kerberos
credential delegation) which was committed on the grounds of concern
about portability, but on further review and discussion, it's clear that
we are better off explicitly requiring MIT Kerberos as that appears to
be the only GSSAPI library currently that's under proper maintenance
and ongoing development.  The API used for storing credentials was added
to MIT Kerberos over a decade ago while for the other libraries which
appear to be mainly based on Heimdal, which exists explicitly to be a
re-implementation of MIT Kerberos, the API never made it to a released
version (even though it was added to the Heimdal git repo over 5 years
ago..).

This post-feature-freeze change was approved by the RMT.

Discussion: https://postgr.es/m/ZDDO6jaESKaBgej0%40tamriel.snowman.net
---
 contrib/dblink/dblink.c   | 127 ---
 contrib/dblink/expected/dblink.out|   4 +-
 contrib/postgres_fdw/connection.c |  72 +++-
 .../postgres_fdw/expected/postgres_fdw.out|  19 +-
 contrib/postgres_fdw/option.c |   6 +
 contrib/postgres_fdw/sql/postgres_fdw.sql |   3 +-
 doc/src/sgml/config.sgml  |  17 +
 doc/src/sgml/dblink.sgml  |   5 +-
 doc/src/sgml/libpq.sgml   |  41 +++
 doc/src/sgml/monitoring.sgml  |   9 +
 doc/src/sgml/postgres-fdw.sgml|   7 +-
 src/backend/catalog/system_views.sql  |   3 +-
 src/backend/foreign/foreign.c |   1 +
 src/backend/libpq/auth.c  |  13 +-
 src/backend/libpq/be-gssapi-common.c  |  53 +++
 src/backend/libpq/be-secure-gssapi.c  |  26 +-
 src/backend/utils/activity/backend_status.c   |   1 +
 src/backend/utils/adt/pgstatfuncs.c   |  21 +-
 src/backend/utils/init/postinit.c |   8 +-
 src/backend/utils/misc/guc_tables.c   |  10 +
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/include/catalog/pg_proc.dat   |   6 +-
 src/include/libpq/auth.h  |   1 +
 src/include/libpq/be-gssapi-common.h  |   3 +
 src/include/libpq/libpq-be.h  |   2 +
 src/include/utils/backend_status.h|   1 +
 src/interfaces/libpq/exports.txt  |   1 +
 src/interfaces/libpq/fe-auth.c|  15 +-
 src/interfaces/libpq/fe-connect.c |  17 +
 src/interfaces/libpq/fe-secure-gssapi.c   |  23 +-
 src/interfaces/libpq/libpq-fe.h   |   1 +
 src/interfaces/libpq/libpq-int.h  |   2 +
 src/test/kerberos/Makefile|   3 +
 src/test/kerberos/t/001_auth.pl   | 331 --
 src/test/perl/PostgreSQL/Test/Utils.pm|  27 ++
 src/test/regress/expected/rules.out   |  11 +-
 36 files changed, 755 insertions(+), 136 deletions(-)

diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 78a8bcee6e..55f75eff36 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -48,6 +48,7 @@
 #include "funcapi.h"
 #include "lib/stringinfo.h"
 #include "libpq-fe.h"
+#include "libpq/libpq-be.h"
 #include "libpq/libpq-be-fe-helpers.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
@@ -111,7 +112,8 @@ static HeapTuple get_tuple_of_interest(Relation rel, int *pkattnums, int pknumat
 static Relation get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclMode aclmode);
 static char *generate_relation_name(Relation rel);
 static void dblink_connstr_check(const char *connstr);
-static void dblink_security_check(PGconn *conn, remoteConn *rconn);
+static bool dblink_connstr_has_pw(const char *connstr);
+static void dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr);
 static void dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
 			 bool fail, const char *fmt,...) pg_attribute_printf(5, 6);
 static char *get_connect_string(const char *servername);
@@ -213,7 +215,7 @@ dblink_get_conn(char *conname_or_str,
 	 errmsg("could not establish connection"),
 	 errdetail_internal("%s", msg)));
 		}
-		dblink_security_check(conn, rconn);
+		dblink_security_check(conn, rconn, connstr);
 		if (PQclientEncoding(conn) != GetDatabaseEncoding())
 			PQsetClientEncoding(conn, GetDatabaseEncodingName());
 		freeconn = true;
@@ -307,7 +309,7 @@ dblink_connect(PG_FUNCTION_ARGS)
 	}
 
 	/* check pas

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Mark Dilger]

> On Apr 12, 2023, at 8:01 AM, Robert Haas  wrote:
> 
> "hey, I'd like that too"

I like the idea in principle.  I have been developing a testing infrastructure 
in my spare time and would rather not duplicate Andrew's TAP logic.  If we get 
this pushed down into the server itself, all the test infrastructure can use a 
single, shared solution.

As for the implementation, I just briefly scanned the patch.  

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Direct I/O

[Dagfinn Ilmari Mannsåker]

Andrew Dunstan  writes:

> On 2023-04-12 We 10:23, Dagfinn Ilmari Mannsåker wrote:
>> Andrew Dunstan  writes:
>>
>>> On 2023-04-12 We 01:48, Thomas Munro wrote:
 On Wed, Apr 12, 2023 at 3:04 PM Thomas Munro   
 wrote:
> On Wed, Apr 12, 2023 at 2:56 PM Christoph Berg   wrote:
>> I'm hitting a panic in t_004_io_direct. The build is running on
>> overlayfs on tmpfs/ext4 (upper/lower) which is probably a weird
>> combination but has worked well for building everything over the last
>> decade. On Debian unstable:
>>
>> PANIC:  could not open file "pg_wal/00010001": Invalid 
>> argument
> ... I have a new idea:  perhaps it is possible to try
> to open a file with O_DIRECT from perl, and if it fails like that,
> skip the test.  Looking into that now.
 I think I have that working OK.  Any Perl hackers want to comment on
 my use of IO::File (copied from examples on the internet that showed
 how to use O_DIRECT)?  I am not much of a perl hacker but according to
 my package manager, IO/File.pm came with perl itself.  And the Fcntl
 eval trick that I copied from File::stat, and the perl-critic
 suppression that requires?
>>>
>>> I think you can probably replace a lot of the magic here by simply saying
>>>
>>>
>>> if (Fcntl->can("O_DIRECT")) ...
>> Fcntl->can() is true for all constants that Fcntl knows about, whether
>> or not they are defined for your OS. `defined &O_DIRECT` is the simplest
>> check, see my other reply to Thomas.
>>
>>
>
> My understanding was that Fcntl only exported constants known to the
> OS. That's certainly what its docco suggests, e.g.:
>
>     By default your system's F_* and O_* constants (eg, F_DUPFD and
> O_CREAT)
>     and the FD_CLOEXEC constant are exported into your namespace.

It's a bit more magical than that (this is Perl after all).  They are
all exported (which implicitly creates stubs visible to `->can()`,
similarly to forward declarations like `sub O_FOO;`), but only the
defined ones (`#ifdef O_FOO` is true) are defined (`defined &O_FOO` is
true).  The rest fall through to an AUTOLOAD¹ function that throws an
exception for undefined ones.

Here's an example (Fcntl knows O_RAW, but Linux does not define it):

$ perl -E '
use strict; use Fcntl;
say "can", main->can("O_RAW") ? "" : "not";
say defined &O_RAW ? "" : "not ", "defined";
say O_RAW;'
can
not defined
Your vendor has not defined Fcntl macro O_RAW, used at -e line 4

While O_DIRECT is defined:

$ perl -E '
use strict; use Fcntl;
say "can", main->can("O_DIRECT") ? "" : "not";
say defined &O_DIRECT ? "" : "not ", "defined";
say O_DIRECT;'
can
defined
16384

And O_FOO is unknown to Fcntl (the parens on `O_FOO()q are to make it
not a bareword, which would be a compile error under `use strict;` when
the sub doesn't exist at all):

$ perl -E '
use strict; use Fcntl;
say "can", main->can("O_FOO") ? "" : "not";
say defined &O_FOO ? "" : "not ", "defined";
say O_FOO();'
cannot
not defined
Undefined subroutine &main::O_FOO called at -e line 4.

> cheers
>
>
> andrew

- ilmari

[1] https://perldoc.perl.org/perlsub#Autoloading

Re: longfin missing gssapi_ext.h

[Daniel Gustafsson]

> On 12 Apr 2023, at 16:55, Stephen Frost  wrote:
> 
> Greetings,
> 
> * Daniel Gustafsson (dan...@yesql.se) wrote:
>>> On 12 Apr 2023, at 16:33, Stephen Frost  wrote:
>>> Sure, reworked that way and attached.
>> 
>> While not changed in this hunk, does the comment regarding Heimdal still 
>> apply?
>> 
>> @@ -918,6 +919,7 @@ pg_GSS_recvauth(Port *port)
>>  int mtype;
>>  StringInfoData buf;
>>  gss_buffer_desc gbuf;
>> +gss_cred_id_t delegated_creds;
>> 
>>  /*
>>   * Use the configured keytab, if there is one.  Unfortunately, Heimdal
> 
> Good catch.  No, it doesn't.  I'm not anxious to actually change that
> code at this point but we could certainly consider changing it in the
> future.  I'll update this comment (and the identical one in
> secure_open_gssapi) accordingly.

Sounds like a good plan.

--
Daniel Gustafsson

Re: longfin missing gssapi_ext.h

[Jonathan S. Katz]

On 4/12/23 12:22 PM, Stephen Frost wrote:

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:

Stephen Frost  writes:

Updated patch set attached.


LGTM


Great, thanks.

I cleaned up the commit messages a bit more and added links to the
discussion.  If there isn't anything more then I'll plan to push these
later today or tomorrow.


Great -- thanks for your attention to this. I'm glad we have an 
opportunity to de-revert (devert?).


Jonathan



OpenPGP_signature
Description: OpenPGP digital signature

Allowing parallel-safe initplans

[Tom Lane]

Pursuant to the discussion at [1], here's a patch that removes our
old restriction that a plan node having initPlans can't be marked
parallel-safe (dating to commit ab77a5a45).  That was really a special
case of the fact that we couldn't transmit subplans to parallel
workers at all.  We fixed that in commit 5e6d8d2bb and follow-ons,
but this case never got addressed.

Along the way, this also takes care of some sloppiness about updating
path costs to match when we move initplans from one place to another
during createplan.c and setrefs.c.  Since all the planning decisions are
already made by that point, this is just cosmetic; but it seems good
to keep EXPLAIN output consistent with where the initplans are.

The diff in query_planner() might be worth remarking on.  I found
that one because after fixing things to allow parallel-safe initplans,
one partition_prune test case changed plans (as shown in the patch)
--- but only when debug_parallel_query was active.  The reason
proved to be that we only bothered to mark Result nodes as potentially
parallel-safe when debug_parallel_query is on.  This neglects the
fact that parallel-safety may be of interest for a sub-query even
though the Result itself doesn't parallelize.

There's only one existing test case that visibly changes plan with
these changes.  The new plan is clearly saner-looking than before,
and testing with some data loaded into the table confirms that it
is faster.  I'm not sure if it's worth devising more test cases.

I'll park this in the July commitfest.

regards, tom lane

[1] https://www.postgresql.org/message-id/flat/ZDVt6MaNWkRDO1LQ%40telsasoft.com

diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 910ffbf1e1..2ec8a537ae 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -6488,6 +6488,8 @@ materialize_finished_plan(Plan *subplan)
 {
 	Plan	   *matplan;
 	Path		matpath;		/* dummy for result of cost_material */
+	Cost		initplan_cost;
+	bool		unsafe_initplans;
 
 	matplan = (Plan *) make_material(subplan);
 
@@ -6495,20 +6497,25 @@ materialize_finished_plan(Plan *subplan)
 	 * XXX horrid kluge: if there are any initPlans attached to the subplan,
 	 * move them up to the Material node, which is now effectively the top
 	 * plan node in its query level.  This prevents failure in
-	 * SS_finalize_plan(), which see for comments.  We don't bother adjusting
-	 * the subplan's cost estimate for this.
+	 * SS_finalize_plan(), which see for comments.
 	 */
 	matplan->initPlan = subplan->initPlan;
 	subplan->initPlan = NIL;
 
+	/* Move the initplans' cost delta, as well */
+	SS_compute_initplan_cost(matplan->initPlan,
+			 &initplan_cost, &unsafe_initplans);
+	subplan->startup_cost -= initplan_cost;
+	subplan->total_cost -= initplan_cost;
+
 	/* Set cost data */
 	cost_material(&matpath,
   subplan->startup_cost,
   subplan->total_cost,
   subplan->plan_rows,
   subplan->plan_width);
-	matplan->startup_cost = matpath.startup_cost;
-	matplan->total_cost = matpath.total_cost;
+	matplan->startup_cost = matpath.startup_cost + initplan_cost;
+	matplan->total_cost = matpath.total_cost + initplan_cost;
 	matplan->plan_rows = subplan->plan_rows;
 	matplan->plan_width = subplan->plan_width;
 	matplan->parallel_aware = false;
diff --git a/src/backend/optimizer/plan/planmain.c b/src/backend/optimizer/plan/planmain.c
index 7afd434c60..fcc0eacd25 100644
--- a/src/backend/optimizer/plan/planmain.c
+++ b/src/backend/optimizer/plan/planmain.c
@@ -112,14 +112,17 @@ query_planner(PlannerInfo *root,
  * quals are parallel-restricted.  (We need not check
  * final_rel->reltarget because it's empty at this point.
  * Anything parallel-restricted in the query tlist will be
- * dealt with later.)  This is normally pretty silly, because
- * a Result-only plan would never be interesting to
- * parallelize.  However, if debug_parallel_query is on, then
- * we want to execute the Result in a parallel worker if
- * possible, so we must do this.
+ * dealt with later.)  We should always do this in a subquery,
+ * since it might be useful to use the subquery in parallel
+ * paths in the parent level.  At top level this is normally
+ * not worth the cycles, because a Result-only plan would
+ * never be interesting to parallelize.  However, if
+ * debug_parallel_query is on, then we want to execute the
+ * Result in a parallel worker if possible, so we must check.
  */
 if (root->glob->parallelModeOK &&
-	debug_parallel_query != DEBUG_PARALLEL_OFF)
+	(root->query_level > 1 ||
+	 debug_parallel_query != DEBUG_PARALLEL_OFF))
 	final_rel->consider_parallel =
 		is_parallel_safe(root, parse->jointree->quals);
 
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 502ccbcea2..e196f06c07 100644
--- a/src/backend/optimizer/

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Jacob Champion]

On Fri, Apr 7, 2023 at 5:07 AM Andrew Dunstan  wrote:
> For TAP tests we have pretty much resolved the port collisions issue for TCP 
> ports too. See commit 9b4eafcaf4

The Cirrus config still has the following for the Windows tests:

# Avoids port conflicts between concurrent tap test runs
PG_TEST_USE_UNIX_SOCKETS: 1

Is that comment out of date, or would this proposal improve the
Windows situation too?

--Jacob

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Jacob Champion]

On Wed, Apr 12, 2023 at 2:24 AM Daniel Gustafsson  wrote:
> > On 12 Apr 2023, at 09:11, Peter Eisentraut 
> >  wrote:
> > #   Failed test 'sslrootcert=system does not connect with private CA: 
> > matches'
> > #   at t/001_ssltests.pl line 479.
> > #   'psql: error: connection to server at "127.0.0.1", port 
> > 53971 failed: SSL SYSCALL error: Undefined error: 0'
> > # doesn't match '(?^:SSL error: certificate verify failed)'
> >
> > This is with OpenSSL 3.1.0 from macOS/Homebrew.
> >
> > If I instead use OpenSSL 1.1.1t, then the tests pass.
>
> I am unable to reproduce this (or any failure) with OpenSSL 3.1 built from
> source (or 3.0 or 3.1.1-dev) or installed via homebrew (on macOS 12 with Intel
> CPU).  Do you have any more clues from logs what might've happened?

This looks similar (but not identical) to the brew bug we're working
around for Cirrus, in which `brew cleanup` breaks the OpenSSL
installation and turns certificate verification failures into
bizarrely unhelpful messages.

Peter, you should have a .../etc/openssl@3/certs directory somewhere
in your Homebrew installation prefix -- do you, or has Homebrew
removed it by mistake?

--Jacob

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Greg Stark]

On Wed, 12 Apr 2023 at 11:02, Robert Haas  wrote:
>
> I mostly just wanted to say that I disagreed with Tom about the
> particular point on postmaster.pid, without really expressing an
> opinion about anything else.

I don't object to using the pid file as the mechanism -- but it is a
bit of an awkward UI for shell scripting. I imagine it would be handy
if pg_ctl had an option to just print the port number so you could get
it with a simple port=`pg_ctl -D  status-port`

-- 
greg

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Robert Haas]

On Wed, Apr 12, 2023 at 1:31 PM Greg Stark  wrote:
> I don't object to using the pid file as the mechanism -- but it is a
> bit of an awkward UI for shell scripting. I imagine it would be handy
> if pg_ctl had an option to just print the port number so you could get
> it with a simple port=`pg_ctl -D  status-port`

That's not a bad idea, and would provide some additional isolation to
reduce direct dependency on the PID file format.

However, awk 'NR==4' $PGDATA/postmaster.pid is hardly insanity. If it
can be done with a 5-character awk script, it's not too hard. The kind
of thing you're talking about is much more important with things like
pg_control or postgresql.conf that have much more complicated formats.
The format of the PID file is intentionally simple. But that's not to
say that I'm objecting.

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

Re: Tab completion for AT TIME ZONE

[Dagfinn Ilmari Mannsåker]

Dagfinn Ilmari Mannsåker  writes:

> Hi hackers,
>
> A while back we added support for completing time zone names after SET
> TIMEZONE, but we failed to do the same for the AT TIME ZONE operator.
> Here's a trivial patch for that.

Added to the 2023-07 commitfest:

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

- ilmari

Re: Adding argument names to aggregate functions

[Dagfinn Ilmari Mannsåker]

Dagfinn Ilmari Mannsåker  writes:

> Hi hackers,
>
> I'm sure I'm not the only one who can never remember which way around
> the value and delimiter arguments go for string_agg() and has to look it
> up in the manual every time. To make it more convenient, here's a patch
> that adds proargnames to its pg_proc entries so that it can be seen with
> a quick \df in psql.

Added to the 2023-07 commitfest:

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

- ilmari

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Tom Lane]

Robert Haas  writes:
> On Wed, Apr 12, 2023 at 1:31 PM Greg Stark  wrote:
>> I don't object to using the pid file as the mechanism -- but it is a
>> bit of an awkward UI for shell scripting. I imagine it would be handy
>> if pg_ctl had an option to just print the port number so you could get
>> it with a simple port=`pg_ctl -D  status-port`

> That's not a bad idea, and would provide some additional isolation to
> reduce direct dependency on the PID file format.

Yeah.  My main concern here is with limiting our ability to change
the pidfile format in future.  If we can keep the dependencies on that
localized to code we control, it'd be much better.

regards, tom lane

Re: Allowing parallel-safe initplans

[Robert Haas]

On Wed, Apr 12, 2023 at 12:44 PM Tom Lane  wrote:
> Pursuant to the discussion at [1], here's a patch that removes our
> old restriction that a plan node having initPlans can't be marked
> parallel-safe (dating to commit ab77a5a45).  That was really a special
> case of the fact that we couldn't transmit subplans to parallel
> workers at all.  We fixed that in commit 5e6d8d2bb and follow-ons,
> but this case never got addressed.

Nice.

> Along the way, this also takes care of some sloppiness about updating
> path costs to match when we move initplans from one place to another
> during createplan.c and setrefs.c.  Since all the planning decisions are
> already made by that point, this is just cosmetic; but it seems good
> to keep EXPLAIN output consistent with where the initplans are.

OK. It would be nicer if we had a more principled approach here, but
that's a job for another day.

> There's only one existing test case that visibly changes plan with
> these changes.  The new plan is clearly saner-looking than before,
> and testing with some data loaded into the table confirms that it
> is faster.  I'm not sure if it's worth devising more test cases.

It seems like it would be nice to see one or two additional scenarios
where these changes bring a benefit, with different kinds of plan
shapes.

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

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Robert Haas]

On Wed, Apr 12, 2023 at 1:56 PM Tom Lane  wrote:
> Yeah.  My main concern here is with limiting our ability to change
> the pidfile format in future.  If we can keep the dependencies on that
> localized to code we control, it'd be much better.

I don't know if it's considered officially supported, but I often use
pg_ctl stop on a directory without worrying about whether I'm doing it
with the same server version that's running in that directory. I'd be
reluctant to break that property. So I bet our ability to modify the
file format is already quite limited.

But again, no issue with having a way for pg_ctl to fish the
information out of there.

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

Re: Wrong results from Parallel Hash Full Join

[Andres Freund]

Hi,

On 2023-04-12 10:57:17 -0400, Melanie Plageman wrote:
> HeapTupleHeaderHasMatch() checks if HEAP_TUPLE_HAS_MATCH is set.
> 
> In htup_details.h, you will see that HEAP_TUPLE_HAS_MATCH is defined as
> HEAP_ONLY_TUPLE
> /*
>  * HEAP_TUPLE_HAS_MATCH is a temporary flag used during hash joins.  It is
>  * only used in tuples that are in the hash table, and those don't need
>  * any visibility information, so we can overlay it on a visibility flag
>  * instead of using up a dedicated bit.
>  */
> #define HEAP_TUPLE_HAS_MATCHHEAP_ONLY_TUPLE /* tuple has a join match */
> 
> If you redefine HEAP_TUPLE_HAS_MATCH as something that isn't already
> used, say 0x1800, the query returns correct results.
> [...]
> The question is, why does this only happen for a parallel full hash join?

I'd guess that PHJ code is missing a HeapTupleHeaderClearMatch() somewhere,
but the non-parallel case isn't.

Greetings,

Andres Freund

testing sepgsql in CI

[Andres Freund]

Hi,

Forking of [1].

On 2023-04-12 14:11:10 -0400, Joe Conway wrote:
> On 4/12/23 13:44, Alvaro Herrera wrote:
> > Revert "Catalog NOT NULL constraints" and fallout
> >
> > This reverts commit e056c557aef4 and minor later fixes thereof.
>
> Seems 76c111a7f1 (as well as some other maybe) needs to be reverted as well.

This reminds me: Is there a chance you could help out trying to make sepgsql
get tested as part of CI?

Currently CI uses only Debian for linux - can sepgsql be made work on that?

Greetings,

Andres Freund

[1] https://postgr.es/m/84b8db57-de05-b413-1826-bc8c9cef85e3%40joeconway.com

Re: postgres_fdw, dblink, and CREATE SUBSCRIPTION security

[Jacob Champion]

On 3/30/23 05:58, Robert Haas wrote:
> On Fri, Mar 24, 2023 at 5:47 PM Jacob Champion  
> wrote:
>> Okay, but this is walking back from the network example you just
>> described upthread. Do you still consider that in scope, or...?
> 
> Sorry, I don't know which example you mean.

The symmetrical proxy situation you described, where all the proxies are
mutually trusting. While it's easier to secure that setup than the
asymmetrical ones, it's also not a localhost-only situation anymore, and
the moment you open up to other machines is where I think your
characterization runs into trouble.

> I guess I wouldn't have a problem blaming the DBA here, but you seem
> to be telling me that the security literature has settled on another
> kind of approach, and I'm not in a position to dispute that. It still
> feels weird to me, though.

If it helps, [1] is a paper that helped me wrap my head around some of
it. It's focused on capability systems and an academic audience, but the
"Avoiding Confused Deputy Problems" section starting on page 11 is a
good place to jump to for the purposes of this discussion.

--Jacob

[1] https://srl.cs.jhu.edu/pubs/SRL2003-02.pdf

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Tom Lane]

Robert Haas  writes:
> On Wed, Apr 12, 2023 at 1:56 PM Tom Lane  wrote:
>> Yeah.  My main concern here is with limiting our ability to change
>> the pidfile format in future.  If we can keep the dependencies on that
>> localized to code we control, it'd be much better.

> I don't know if it's considered officially supported, but I often use
> pg_ctl stop on a directory without worrying about whether I'm doing it
> with the same server version that's running in that directory. I'd be
> reluctant to break that property. So I bet our ability to modify the
> file format is already quite limited.

IMO, the only aspect we consider "officially supported" for outside
use is that the first line contains the postmaster's PID.  All the
rest is private (and has changed as recently as v10).  Without
having actually checked the code, I think that "pg_ctl stop" relies
only on that aspect, or at least it could be made to do so at need.
So I think your example would survive other changes in the format.

I don't really want external code knowing that line 4 is the port,
because I foresee us breaking that someday --- what will happen
when we want to allow one postmaster to support multiple ports?
Maybe we'll decide that we don't have to reflect that in the
pidfile, but let's not constrain our decisions ahead of time.

regards, tom lane

Re: postgres_fdw, dblink, and CREATE SUBSCRIPTION security

[Jacob Champion]

On 3/30/23 11:13, Stephen Frost wrote:
>> Okay, but this is walking back from the network example you just
>> described upthread. Do you still consider that in scope, or...?
> 
> The concern around the network certainly needs to be in-scope overall.

Sounds good!

> Who are we trusting with what?  In particular, I'd argue that the user
> who is able to install the postgres_fdw extension and the user who is
> able to issue the CREATE SERVER are largely trusted; at least in so far
> as the user doing CREATE SERVER is allowed to create the server and
> through that allowed to make outbound connections from the Proxy.
> 
> Therefore, the Proxy is configured with postgres_fdw and with a trusted
> user performing the CREATE SERVER.
> 
> What doesn't this handle today?  Connection side-effects are one
> problem- once the CREATE SERVER is done, any user with USAGE rights on
> the server can create a USER MAPPING for themselves, either with a
> password or without one (if they're able to proxy GSS credentials to the
> system).  They aren't able to set password_required though, which
> defaults to true.  However, without having require_auth set, they're
> able to cause the Proxy to reach an authentication stage with the Target
> that might not match what credentials they're supposed to be providing.
> 
> We attempt to address this by checking post-auth to Target that we used
> the credentials to connect that we expected to- if GSS credentials were
> proxied, then we expect to use those.  If a password was provided then
> we expect to use a password to auth (only checked after we see if GSS
> credentials were proxied and used).  The issue here is 'post-auth' bit,
> we'd prefer to fail the connection pre-auth if it isn't what we're
> expecting.

Right. Keep in mind that require_auth is post-auth, though; it can't fix
that issue, so it doesn't fix any connection side-effect problems at all.

> Should we then explicit set require_auth=gss when GSS
> credentials are proxied?  Also, if a password is provided, then
> explicitly set require_auth=scram-sha-256?  Or default to these, at
> least, and allow the CREATE SERVER user to override our choices?  Or
> should it be a USER MAPPING option that's restricted?  Or not?
IMO, yes -- whatever credentials the proxy is forwarding from the user,
the proxy should be checking that the server has actually used them. The
person with the ability to create a USER MAPPING should probably not
have the ability to override that check.

>>> I think that what you're proposing is that B and C can just be allowed
>>> to proxy to A and A can say "hey, by the way, I'm just gonna let you
>>> in without asking for anything else" and B and C can, when proxying,
>>> react to that by disconnecting before the connection actually goes
>>> through. That's simpler, in a sense. It doesn't require us to set up
>>> the proxy configuration on B and C in a way that matches what
>>> pg_hba.conf allows on A. Instead, B and C can automatically deduce
>>> what connections they should refuse to proxy.
>>
>> Right. It's meant to take the "localhost/wraparound connection" out of a
>> class of special things we have to worry about, and make it completely
>> boring.
> 
> Again, trying to get at a more concrete example- the concern here is a
> user with CREATE SERVER ability could leverage that access to become a
> superuser if the system is configured with 'peer' access, right?

Or 'trust localhost', or 'ident [postgres user]', yes.

> A
> non-superuser is already prevented from being able to set
> "password_required=false", perhaps we shouldn't allow them to set
> "require_auth=none" (or have that effect) either?

I think that sounds reasonable.

> Perhaps the system
> should simply forcibly set require_auth based on the credentials
> provided in the USER MAPPING or on the connection and have require_auth
> otherwise restricted to superuser (who could override it if they'd
> really like to)?  Perhaps if password_required=false we implicitly
> un-set require_auth, to avoid having to make superusers change their
> existing configurations where they've clearly already accepted that
> credential-less connections are allowed.

Mm, I think I like the first idea better. If you've set a password,
wouldn't you like to know if the server ignored it? If password_required
is false, *and* you don't have a password, then we can drop require_auth
without issue.

> Automatically setting require_auth and restricting the ability of it to
> be set on user mappings to superusers doesn't strike me as terribly
> difficult to do and seems like it'd prevent this concern.
> 
> Just to make sure I'm following- Robert's up-thread suggestion of an
> 'outbound pg_hba' would be an additional restriction when it comes to
> what a user who can use CREATE SERVER is allowed to do?

Yes. That can provide additional safety in the case where you really
need to take the require_auth checks away for whatever reason. I think
it's just a good in-depth measur

Re: testing sepgsql in CI

[Joe Conway]

On 4/12/23 14:17, Andres Freund wrote:

Hi,

Forking of [1].

On 2023-04-12 14:11:10 -0400, Joe Conway wrote:

On 4/12/23 13:44, Alvaro Herrera wrote:
> Revert "Catalog NOT NULL constraints" and fallout
>
> This reverts commit e056c557aef4 and minor later fixes thereof.

Seems 76c111a7f1 (as well as some other maybe) needs to be reverted as well.


This reminds me: Is there a chance you could help out trying to make sepgsql
get tested as part of CI?

Currently CI uses only Debian for linux - can sepgsql be made work on that?


Theoretically selinux can be made to work on Debian, but I only have 
one, failed, past attempt at making it work ;-)


I can certainly try to help though.

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

Re: Direct I/O

[Andrew Dunstan]

On 2023-04-12 We 12:38, Dagfinn Ilmari Mannsåker wrote:

Andrew Dunstan  writes:


On 2023-04-12 We 10:23, Dagfinn Ilmari Mannsåker wrote:

Andrew Dunstan   writes:


On 2023-04-12 We 01:48, Thomas Munro wrote:

On Wed, Apr 12, 2023 at 3:04 PM Thomas Munrowrote:

On Wed, Apr 12, 2023 at 2:56 PM Christoph Bergwrote:

I'm hitting a panic in t_004_io_direct. The build is running on
overlayfs on tmpfs/ext4 (upper/lower) which is probably a weird
combination but has worked well for building everything over the last
decade. On Debian unstable:

PANIC:  could not open file "pg_wal/00010001": Invalid argument

... I have a new idea:  perhaps it is possible to try
to open a file with O_DIRECT from perl, and if it fails like that,
skip the test.  Looking into that now.

I think I have that working OK.  Any Perl hackers want to comment on
my use of IO::File (copied from examples on the internet that showed
how to use O_DIRECT)?  I am not much of a perl hacker but according to
my package manager, IO/File.pm came with perl itself.  And the Fcntl
eval trick that I copied from File::stat, and the perl-critic
suppression that requires?

I think you can probably replace a lot of the magic here by simply saying


if (Fcntl->can("O_DIRECT")) ...

Fcntl->can() is true for all constants that Fcntl knows about, whether
or not they are defined for your OS. `defined &O_DIRECT` is the simplest
check, see my other reply to Thomas.



My understanding was that Fcntl only exported constants known to the
OS. That's certainly what its docco suggests, e.g.:

     By default your system's F_* and O_* constants (eg, F_DUPFD and
O_CREAT)
     and the FD_CLOEXEC constant are exported into your namespace.

It's a bit more magical than that (this is Perl after all).  They are
all exported (which implicitly creates stubs visible to `->can()`,
similarly to forward declarations like `sub O_FOO;`), but only the
defined ones (`#ifdef O_FOO` is true) are defined (`defined &O_FOO` is
true).  The rest fall through to an AUTOLOAD¹ function that throws an
exception for undefined ones.

Here's an example (Fcntl knows O_RAW, but Linux does not define it):

 $ perl -E '
 use strict; use Fcntl;
 say "can", main->can("O_RAW") ? "" : "not";
 say defined &O_RAW ? "" : "not ", "defined";
 say O_RAW;'
 can
 not defined
 Your vendor has not defined Fcntl macro O_RAW, used at -e line 4

While O_DIRECT is defined:

 $ perl -E '
 use strict; use Fcntl;
 say "can", main->can("O_DIRECT") ? "" : "not";
 say defined &O_DIRECT ? "" : "not ", "defined";
 say O_DIRECT;'
 can
 defined
 16384

And O_FOO is unknown to Fcntl (the parens on `O_FOO()q are to make it
not a bareword, which would be a compile error under `use strict;` when
the sub doesn't exist at all):

 $ perl -E '
 use strict; use Fcntl;
 say "can", main->can("O_FOO") ? "" : "not";
 say defined &O_FOO ? "" : "not ", "defined";
 say O_FOO();'
 cannot
 not defined
 Undefined subroutine &main::O_FOO called at -e line 4.




*grumble* a bit too magical for my taste. Thanks for the correction.


cheers


andrew

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

Re: Wrong results from Parallel Hash Full Join

[Melanie Plageman]

On Wed, Apr 12, 2023 at 2:14 PM Andres Freund  wrote:
>
> Hi,
>
> On 2023-04-12 10:57:17 -0400, Melanie Plageman wrote:
> > HeapTupleHeaderHasMatch() checks if HEAP_TUPLE_HAS_MATCH is set.
> >
> > In htup_details.h, you will see that HEAP_TUPLE_HAS_MATCH is defined as
> > HEAP_ONLY_TUPLE
> > /*
> >  * HEAP_TUPLE_HAS_MATCH is a temporary flag used during hash joins.  It is
> >  * only used in tuples that are in the hash table, and those don't need
> >  * any visibility information, so we can overlay it on a visibility flag
> >  * instead of using up a dedicated bit.
> >  */
> > #define HEAP_TUPLE_HAS_MATCHHEAP_ONLY_TUPLE /* tuple has a join match */
> >
> > If you redefine HEAP_TUPLE_HAS_MATCH as something that isn't already
> > used, say 0x1800, the query returns correct results.
> > [...]
> > The question is, why does this only happen for a parallel full hash join?
>
> I'd guess that PHJ code is missing a HeapTupleHeaderClearMatch() somewhere,
> but the non-parallel case isn't.

Indeed. Thanks! This diff fixes the case Richard provided.

diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index a45bd3a315..54c06c5eb3 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -1724,6 +1724,7 @@ retry:
/* Store the hash value in the HashJoinTuple header. */
hashTuple->hashvalue = hashvalue;
memcpy(HJTUPLE_MINTUPLE(hashTuple), tuple, tuple->t_len);
+   HeapTupleHeaderClearMatch(HJTUPLE_MINTUPLE(hashTuple));

/* Push it onto the front of the bucket's list */
ExecParallelHashPushTuple(&hashtable->buckets.shared[bucketno],

I will propose a patch that includes this change and a test.

I just want to convince myself that ExecParallelHashTableInsertCurrentBatch()
covers the non-batch 0 cases and we don't need to add something to
sts_puttuple().

- Melanie

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Peter Eisentraut]

On 12.04.23 18:54, Jacob Champion wrote:

Peter, you should have a .../etc/openssl@3/certs directory somewhere
in your Homebrew installation prefix -- do you, or has Homebrew
removed it by mistake?


I don't have that, but I don't have it for openssl@1.1 either.  I have

~$ ll /usr/local/etc/openssl@3
total 76
drwxr-xr-x 7 peter admin   224 2023-03-08 08:49 misc/
lrwxr-xr-x 1 peter admin27 2023-03-21 13:41 cert.pem -> 
../ca-certificates/cert.pem

-rw-r--r-- 1 peter admin   412 2023-03-21 13:41 ct_log_list.cnf
-rw-r--r-- 1 peter admin   412 2023-03-21 13:41 ct_log_list.cnf.dist
-rw-r--r-- 1 peter admin   351 2023-03-08 08:57 fipsmodule.cnf
-rw-r--r-- 1 peter admin 12386 2023-03-13 10:49 openssl.cnf
-rw-r--r-- 1 peter admin 12292 2023-03-21 13:41 openssl.cnf.default
-rw-r--r-- 1 peter admin 12292 2023-03-08 08:49 openssl.cnf.dist
-rw-r--r-- 1 peter admin 12292 2023-03-21 13:41 openssl.cnf.dist.default

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Daniel Gustafsson]

> On 12 Apr 2023, at 21:43, Peter Eisentraut 
>  wrote:
> 
> On 12.04.23 18:54, Jacob Champion wrote:
>> Peter, you should have a .../etc/openssl@3/certs directory somewhere
>> in your Homebrew installation prefix -- do you, or has Homebrew
>> removed it by mistake?
> 
> I don't have that, but I don't have it for openssl@1.1 either.

The important bit is that your OPENSSLDIR points to a directory which has the
content OpenSSL needs.

> I have
> 
> ~$ ll /usr/local/etc/openssl@3
> total 76
> drwxr-xr-x 7 peter admin   224 2023-03-08 08:49 misc/
> lrwxr-xr-x 1 peter admin27 2023-03-21 13:41 cert.pem -> 
> ../ca-certificates/cert.pem
> -rw-r--r-- 1 peter admin   412 2023-03-21 13:41 ct_log_list.cnf
> -rw-r--r-- 1 peter admin   412 2023-03-21 13:41 ct_log_list.cnf.dist
> -rw-r--r-- 1 peter admin   351 2023-03-08 08:57 fipsmodule.cnf
> -rw-r--r-- 1 peter admin 12386 2023-03-13 10:49 openssl.cnf
> -rw-r--r-- 1 peter admin 12292 2023-03-21 13:41 openssl.cnf.default
> -rw-r--r-- 1 peter admin 12292 2023-03-08 08:49 openssl.cnf.dist
> -rw-r--r-- 1 peter admin 12292 2023-03-21 13:41 openssl.cnf.dist.default

Assuming that's your OPENSSLDIR, then that looks like it should (it's precisely
what I have).

Just to further rule out any issues in the installation, If you run the command
from upthread, does that properly verify postgresql.org?

echo Q | openssl@3/bin/openssl s_client -connect postgresql.org:443 
-verify_return_error

Is the failure repeatable enough that you might be able to tease something out
of the log?  I've been trying again today but been unable to reproduce this =(

We don't have great coverage of macOS in the buildfarm sadly, I wonder if can
get sifaka to run the SSL tests if we ask nicely?

--
Daniel Gustafsson

Re: Temporary tables versus wraparound... again

[Greg Stark]

On Wed, 5 Apr 2023 at 13:42, Andres Freund  wrote:
>
> ISTM that the problem here really is that you're relying on RecentXmin, rather
> than computing something more accurate. Why not use
> GetOldestNonRemovableTransactionId(rel) - It's a bit more expensive, but I
> don't think it'll matter compared to the cost of truncating the relation.

I'm trying to wrap my head around GetOldestNonRemovableTransactionId()
and whether it's the right thing here. This comment is not helping me:

/*
 * Return the oldest XID for which deleted tuples must be preserved in the
 * passed table.
 *
 * If rel is not NULL the horizon may be considerably more recent than
 * otherwise (i.e. fewer tuples will be removable). In the NULL case a horizon
 * that is correct (but not optimal) for all relations will be returned.
 *
 * This is used by VACUUM to decide which deleted tuples must be preserved in
 * the passed in table.
 */


Am I crazy or is the parenthetical comment there exactly backwards? If
the horizon is *more recent* then fewer tuples are *non*-removable.
I.e. *more* tuples are removable, no?

-- 
greg

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Tom Lane]

Daniel Gustafsson  writes:
> We don't have great coverage of macOS in the buildfarm sadly, I wonder if can
> get sifaka to run the SSL tests if we ask nicely?

I was just looking into that, but it seems like it'd be a mess.

I have a modern openssl installation from MacPorts, but if
I try to select that I am going to end up compiling with
-I/opt/local/include -L/opt/local/lib, which exposes all of the
metric buttload of stuff that MacPorts tends to pull in.  sifaka
is intended to test in a reasonably-default macOS environment,
and that would be far from it.

Plausible alternatives include:

1. Hand-built private copy of openssl.  longfin is set up that way,
but I'm not really eager to duplicate that approach, especially if
we want to test cutting-edge openssl.

2. Run a second BF animal that's intentionally pointed at the MacPorts
environment, in hopes of testing what MacPorts users would see.

#2 feels like it might not be a waste of cycles, and certainly that
machine is underworked at the moment.  Thoughts?

regards, tom lane

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Daniel Gustafsson]

> On 12 Apr 2023, at 22:23, Tom Lane  wrote:
> 
> Daniel Gustafsson  writes:
>> We don't have great coverage of macOS in the buildfarm sadly, I wonder if can
>> get sifaka to run the SSL tests if we ask nicely?
> 
> I was just looking into that, but it seems like it'd be a mess.
> 
> I have a modern openssl installation from MacPorts, but if
> I try to select that I am going to end up compiling with
> -I/opt/local/include -L/opt/local/lib, which exposes all of the
> metric buttload of stuff that MacPorts tends to pull in.  sifaka
> is intended to test in a reasonably-default macOS environment,
> and that would be far from it.

That makes sense.

> Plausible alternatives include:
> 
> 1. Hand-built private copy of openssl.  longfin is set up that way,
> but I'm not really eager to duplicate that approach, especially if
> we want to test cutting-edge openssl.
> 
> 2. Run a second BF animal that's intentionally pointed at the MacPorts
> environment, in hopes of testing what MacPorts users would see.
> 
> #2 feels like it might not be a waste of cycles, and certainly that
> machine is underworked at the moment.  Thoughts?

I think #2 would be a good addition.  Most won't build OpenSSL themselves so
seeing builds from envs that are reasonable to expect in the wild is more
interesting.

--
Daniel Gustafsson

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Jacob Champion]

(Peter, your emails are being redirected to spam for me, FYI.
Something about messagingengine.)

On Wed, Apr 12, 2023 at 12:57 PM Daniel Gustafsson  wrote:
> > On 12 Apr 2023, at 21:43, Peter Eisentraut 
> >  wrote:
> > On 12.04.23 18:54, Jacob Champion wrote:
> >> Peter, you should have a .../etc/openssl@3/certs directory somewhere
> >> in your Homebrew installation prefix -- do you, or has Homebrew
> >> removed it by mistake?
> >
> > I don't have that, but I don't have it for openssl@1.1 either.

AFAIK this behavior started with 3.x.

> The important bit is that your OPENSSLDIR points to a directory which has the
> content OpenSSL needs.
>
> > I have
> >
> > ~$ ll /usr/local/etc/openssl@3
> > total 76
> > drwxr-xr-x 7 peter admin   224 2023-03-08 08:49 misc/
> > lrwxr-xr-x 1 peter admin27 2023-03-21 13:41 cert.pem -> 
> > ../ca-certificates/cert.pem
> > -rw-r--r-- 1 peter admin   412 2023-03-21 13:41 ct_log_list.cnf
> > -rw-r--r-- 1 peter admin   412 2023-03-21 13:41 ct_log_list.cnf.dist
> > -rw-r--r-- 1 peter admin   351 2023-03-08 08:57 fipsmodule.cnf
> > -rw-r--r-- 1 peter admin 12386 2023-03-13 10:49 openssl.cnf
> > -rw-r--r-- 1 peter admin 12292 2023-03-21 13:41 openssl.cnf.default
> > -rw-r--r-- 1 peter admin 12292 2023-03-08 08:49 openssl.cnf.dist
> > -rw-r--r-- 1 peter admin 12292 2023-03-21 13:41 openssl.cnf.dist.default
>
> Assuming that's your OPENSSLDIR, then that looks like it should (it's 
> precisely
> what I have).

It surprises me that you can get a successful test with a missing
certs directory. If I remove the workaround in Cirrus, I get the
following error, which looks the same to me:

[20:40:00.253](0.000s) not ok 121 - sslrootcert=system does not
connect with private CA: matches
[20:40:00.253](0.000s) #   Failed test 'sslrootcert=system does
not connect with private CA: matches'
#   at /Users/admin/pgsql/src/test/ssl/t/001_ssltests.pl line 479.
[20:40:00.253](0.000s) #   'psql: error:
connection to server at "127.0.0.1", port 57681 failed: SSL SYSCALL
error: Undefined error: 0'
# doesn't match '(?^:SSL error: certificate verify failed)'

(That broken error message has changed since 3.0; now it's busted in a
new way as of 3.1, I guess.)

Does the test start passing if you create an empty certs directory? It
still wouldn't explain why Daniel's setup is succeeding...

--Jacob

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Tom Lane]

Oh!  I was a little behind on MacPorts updates, and after
pulling the latest (taking their openssl from 3.0.8 to 3.1.0)
I can duplicate Peter's problem:

# +++ tap check in src/test/ssl +++
t/001_ssltests.pl .. 120/? 
#   Failed test 'sslrootcert=system does not connect with private CA: matches'
#   at t/001_ssltests.pl line 479.
#   'psql: error: connection to server at "127.0.0.1", port 
58910 failed: SSL SYSCALL error: Undefined error: 0'
# doesn't match '(?^:SSL error: certificate verify failed)'
t/001_ssltests.pl .. 196/? # Looks like you failed 1 test of 205.
t/001_ssltests.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/205 subtests 
t/002_scram.pl . ok
t/003_sslinfo.pl ... ok

Test Summary Report
---
t/001_ssltests.pl (Wstat: 256 Tests: 205 Failed: 1)
  Failed test:  121
  Non-zero exit status: 1
Files=3, Tests=247, 14 wallclock secs ( 0.02 usr  0.01 sys +  2.04 cusr  1.54 
csys =  3.61 CPU)
Result: FAIL
make: *** [check] Error 1

So whatever this is, it's not strictly Homebrew's issue.

regards, tom lane

Re: Documentation for building with meson

[Peter Eisentraut]

On 11.04.23 19:18, Andres Freund wrote:

Subject: [PATCH v9 2/5] Add data layout options sub-section in installation
  docs

This commit separates out blocksize, segsize and wal_blocksize
options into a separate Data layout options sub-section in both
the make and meson docs. They were earlier in a miscellaneous
section which included several unrelated options. This change
also helps reduce the repetition of the warnings that changing
these parameters breaks on-disk compatibility.


Makes sense. I'm planning to apply this unless Peter or somebody else has
further feedback.


I'm okay with patches 0001 through 0004.

I don't like 0005.  I think we should drop that for now and maybe have a 
separate discussion under a separate heading about that.






 From 11d82aa49efb3d1cbc08f14562a757f115053c8b Mon Sep 17 00:00:00 2001
From: Samay Sharma 
Date: Mon, 13 Feb 2023 16:23:52 -0800
Subject: [PATCH v9 3/5] Remove Anti-Features section from Installation from
  source docs

Currently, several meson setup options are listed in anti-features.
However, they are similar to most other options in the postgres
features list as they are 'auto' features themselves. Also, other
options are likely better suited to the developer options section.
This commit, therefore, moves the options listed in the anti-features
section into other sections and removes that section.

For consistency, this reorganization has been done on the make section
of the docs as well.


Makes sense to me. "Anti-Features" is confusing as a name to start with.

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Peter Eisentraut]

On 12.04.23 22:52, Jacob Champion wrote:

It surprises me that you can get a successful test with a missing
certs directory. If I remove the workaround in Cirrus, I get the
following error, which looks the same to me:

 [20:40:00.253](0.000s) not ok 121 - sslrootcert=system does not
connect with private CA: matches
 [20:40:00.253](0.000s) #   Failed test 'sslrootcert=system does
not connect with private CA: matches'
 #   at /Users/admin/pgsql/src/test/ssl/t/001_ssltests.pl line 479.
 [20:40:00.253](0.000s) #   'psql: error:
connection to server at "127.0.0.1", port 57681 failed: SSL SYSCALL
error: Undefined error: 0'
 # doesn't match '(?^:SSL error: certificate verify failed)'

(That broken error message has changed since 3.0; now it's busted in a
new way as of 3.1, I guess.)

Does the test start passing if you create an empty certs directory? It
still wouldn't explain why Daniel's setup is succeeding...


After

mkdir /usr/local/etc/openssl@3/certs

the tests pass!

Re: v12: ERROR: subplan "InitPlan 2 (returns $4)" was not initialized

[David Rowley]

On Thu, 13 Apr 2023 at 00:34, Tom Lane  wrote:
> Thanks for looking at this.  After sleeping on it, I'm inclined
> to use the v1 patch in the back branches and do the cost fixups
> only in HEAD.

I'm also fine with v1 for the back branches.

David

more elogs hit by sqlsmith (Re: amvalidate(): cache lookup failed for operator class 123)

[Justin Pryzby]

On Mon, Feb 13, 2023 at 07:50:53AM -0600, Justin Pryzby wrote:
> On Thu, May 13, 2021 at 12:01:22PM -0500, Justin Pryzby wrote:
> > postgres=# select amvalidate(123);
> > ERROR:  cache lookup failed for operator class 123
> > The usual expectation is that sql callable functions should return null 
> > rather
> > than hitting elog().
> 
> On Thu, May 13, 2021 at 2:22 PM Tom Lane  wrote:
> > Meh.  I'm not convinced that that position ought to apply to amvalidate.
> 
> On Thu, May 13, 2021 at 04:12:10PM -0400, Robert Haas wrote:
> > I am still of the opinion that we ought to apply it across the board,
> > for consistency. It makes it easier for humans to know which problems
> > are known to be reachable and which are thought to be can't-happen and
> > thus bugs. If we fix cases like this to return a real error code, then
> > anything that comes up as XX000 is likely to be a real bug, whereas if
> > we don't, the things that we're not concerned about have to be
> > filtered out by some other method, probably involving a human being.
> > If the filter that human being has to apply further involves reading
> > Tom Lane's mind and knowing what he will think about a particular
> > report, or alternatively asking him, it just makes complicated
> > something that we could have made simple.
> 
> FWIW, here are some other cases from sqlsmith which hit elog()/XX000:
> 
> postgres=# select unknownin('');
> ERROR:  failed to find conversion function from unknown to text
> postgres=# \errverbose
> ERROR:  XX000: failed to find conversion function from unknown to text
> LOCATION:  coerce_type, parse_coerce.c:542
> 
> postgres=# SELECT pg_catalog.interval( '12 seconds'::interval ,3);
> ERROR:  unrecognized interval typmod: 3
> 
> postgres=# SELECT pg_describe_object(1,0,1);
> ERROR:  invalid non-zero objectSubId for object class 1
> 
> postgres=# SELECT pg_read_file( repeat('a',333));
> ERROR:  could not open file 
> "aa
>  
> aaa"
>  for reading: File name too long

postgres=# SELECT acldefault('a',0);
ERROR:  unrecognized object type abbreviation: a

postgres=# SELECT setweight('a','1');
ERROR:  unrecognized weight: 49

regression=# select float8_regr_intercept(ARRAY[1]) ;
ERROR:  float8_regr_intercept: expected 6-element float8 array

None of these is new in v16.

Re: Parallel Full Hash Join

[Thomas Munro]

On Mon, Apr 10, 2023 at 11:33 AM Michael Paquier  wrote:
> On Sat, Apr 08, 2023 at 02:19:54PM -0400, Melanie Plageman wrote:
> > Another worker attached to the batch barrier, saw that it was in
> > PHJ_BATCH_SCAN, marked it done and detached. This is fine.
> > BarrierArriveAndDetachExceptLast() is meant to ensure no one waits
> > (deadlock hazard) and that at least one worker stays to do the unmatched
> > scan. It doesn't hurt anything for another worker to join and find out
> > there is no work to do.
> >
> > We should simply delete this assertion.

Agreed, and pushed.  Thanks!

> I have added an open item about that.  This had better be tracked.

Thanks, will update.

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Tom Lane]

Peter Eisentraut  writes:
> On 12.04.23 22:52, Jacob Champion wrote:
>> Does the test start passing if you create an empty certs directory? It
>> still wouldn't explain why Daniel's setup is succeeding...

> After
> mkdir /usr/local/etc/openssl@3/certs
> the tests pass!

Likewise, though MacPorts unsurprisingly uses a different place:

$ openssl info -configdir
/opt/local/libexec/openssl3/etc/openssl
$ sudo mkdir /opt/local/libexec/openssl3/etc/openssl/certs
$ make check PG_TEST_EXTRA=ssl
... success!

So this smells to me like a new OpenSSL bug: they should tolerate
a missing certs dir like they used to.  Who wants to file it?

regards, tom lane

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Daniel Gustafsson]

> On 12 Apr 2023, at 23:40, Tom Lane  wrote:
> 
> Peter Eisentraut  writes:
>> On 12.04.23 22:52, Jacob Champion wrote:
>>> Does the test start passing if you create an empty certs directory? It
>>> still wouldn't explain why Daniel's setup is succeeding...
> 
>> After
>> mkdir /usr/local/etc/openssl@3/certs
>> the tests pass!
> 
> Likewise, though MacPorts unsurprisingly uses a different place:
> 
> $ openssl info -configdir
> /opt/local/libexec/openssl3/etc/openssl
> $ sudo mkdir /opt/local/libexec/openssl3/etc/openssl/certs
> $ make check PG_TEST_EXTRA=ssl
> ... success!
> 
> So this smells to me like a new OpenSSL bug: they should tolerate
> a missing certs dir like they used to.  Who wants to file it?

They are specifying that: "A missing default location is still treated as a
success".  That leaves out the interesting bit of what a success means here,
and how it should work when verifications are requested.  That being said, the
same is written in the 1.1.1 manpage.

--
Daniel Gustafsson

Re: Wrong results from Parallel Hash Full Join

[Melanie Plageman]

On Wed, Apr 12, 2023 at 2:59 PM Melanie Plageman
 wrote:
>
> On Wed, Apr 12, 2023 at 2:14 PM Andres Freund  wrote:
> >
> > Hi,
> >
> > On 2023-04-12 10:57:17 -0400, Melanie Plageman wrote:
> > > HeapTupleHeaderHasMatch() checks if HEAP_TUPLE_HAS_MATCH is set.
> > >
> > > In htup_details.h, you will see that HEAP_TUPLE_HAS_MATCH is defined as
> > > HEAP_ONLY_TUPLE
> > > /*
> > >  * HEAP_TUPLE_HAS_MATCH is a temporary flag used during hash joins.  It is
> > >  * only used in tuples that are in the hash table, and those don't need
> > >  * any visibility information, so we can overlay it on a visibility flag
> > >  * instead of using up a dedicated bit.
> > >  */
> > > #define HEAP_TUPLE_HAS_MATCHHEAP_ONLY_TUPLE /* tuple has a join match 
> > > */
> > >
> > > If you redefine HEAP_TUPLE_HAS_MATCH as something that isn't already
> > > used, say 0x1800, the query returns correct results.
> > > [...]
> > > The question is, why does this only happen for a parallel full hash join?
> >
> > I'd guess that PHJ code is missing a HeapTupleHeaderClearMatch() somewhere,
> > but the non-parallel case isn't.
>
> Indeed. Thanks! This diff fixes the case Richard provided.
>
> diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
> index a45bd3a315..54c06c5eb3 100644
> --- a/src/backend/executor/nodeHash.c
> +++ b/src/backend/executor/nodeHash.c
> @@ -1724,6 +1724,7 @@ retry:
> /* Store the hash value in the HashJoinTuple header. */
> hashTuple->hashvalue = hashvalue;
> memcpy(HJTUPLE_MINTUPLE(hashTuple), tuple, tuple->t_len);
> +   HeapTupleHeaderClearMatch(HJTUPLE_MINTUPLE(hashTuple));
>
> /* Push it onto the front of the bucket's list */
> 
> ExecParallelHashPushTuple(&hashtable->buckets.shared[bucketno],
>
> I will propose a patch that includes this change and a test.
>
> I just want to convince myself that ExecParallelHashTableInsertCurrentBatch()
> covers the non-batch 0 cases and we don't need to add something to
> sts_puttuple().

So, indeed, tuples in batches after batch 0 already had their match bit
cleared by ExecParallelHashTableInsertCurrentBatch().

Attached patch includes the fix for ExecParallelHashTableInsert() as
well as a test. I toyed with adapting one of the existing parallel full
hash join tests to cover this case, however, I think Richard's repro is
much more clear. Maybe it is worth throwing in a few updates to the
tables in the existing queries to provide coverage for the other
HeapTupleHeaderClearMatch() calls in the code, though.

- Melanie
From 6aa53dae62e8a8a34166ed6f28ee621cf727c004 Mon Sep 17 00:00:00 2001
From: Melanie Plageman 
Date: Wed, 12 Apr 2023 17:16:28 -0400
Subject: [PATCH v1] Reset PHJ tuple match bit upon hashtable insert

We reuse a single bit to indicate that a hash join tuple is heap-only
and that it has a match during hash join execution. Serial hash join and
parallel multi-batch hash join cleared this bit upon inserting the tuple
into the hashtable. Single batch parallel hash join and batch 0 of
unexpected multi-batch hash joins forgot to do this. Fix that. We didn't
notice before because parallel hash join wasn't using the match bits for
tuples in the hashtable since right and full parallel hash join were
unsupported.

Reported-by: Richard Guo

Discussion: https://postgr.es/m/flat/CAMbWs48Nde1Mv%3DBJv6_vXmRKHMuHZm2Q_g4F6Z3_pn%2B3EV6BGQ%40mail.gmail.com
---
 src/backend/executor/nodeHash.c |  1 +
 src/test/regress/expected/join_hash.out | 20 
 src/test/regress/sql/join_hash.sql  | 17 -
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index a45bd3a315..54c06c5eb3 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -1724,6 +1724,7 @@ retry:
 		/* Store the hash value in the HashJoinTuple header. */
 		hashTuple->hashvalue = hashvalue;
 		memcpy(HJTUPLE_MINTUPLE(hashTuple), tuple, tuple->t_len);
+		HeapTupleHeaderClearMatch(HJTUPLE_MINTUPLE(hashTuple));
 
 		/* Push it onto the front of the bucket's list */
 		ExecParallelHashPushTuple(&hashtable->buckets.shared[bucketno],
diff --git a/src/test/regress/expected/join_hash.out b/src/test/regress/expected/join_hash.out
index 09376514bb..c4f4e2ee54 100644
--- a/src/test/regress/expected/join_hash.out
+++ b/src/test/regress/expected/join_hash.out
@@ -955,6 +955,26 @@ $$);
 (1 row)
 
 rollback to settings;
+-- Ensure that hash join tuple match bits have been cleared before putting them
+-- into the hashtable.
+SAVEPOINT settings;
+SET enable_parallel_hash = on;
+SET min_parallel_table_scan_size = 0;
+SET parallel_setup_cost = 0;
+SET parallel_tuple_cost = 0;
+CREATE TABLE hjtest_matchbits_t1(id int);
+CREATE TABLE hjtest_matchbits_t2(id int);
+INSERT INTO hjtest_matchbits_t1 VALUES (1);
+INSERT INTO hjtest_matchbits_t2 VALUES (2);
+UPDATE hjtest_matchbits_t2 s

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Michael Paquier]

On Wed, Apr 12, 2023 at 02:24:30PM -0400, Tom Lane wrote:
> I don't really want external code knowing that line 4 is the port,
> because I foresee us breaking that someday --- what will happen
> when we want to allow one postmaster to support multiple ports?
> Maybe we'll decide that we don't have to reflect that in the
> pidfile, but let's not constrain our decisions ahead of time.

In the same fashion as something mentioned upthread, the format
portability would not matter much if all the information from the PID
file is wrapped around a pg_ctl command or something equivalent that
controls its output format, say: 
pg_ctl -D $PGDATA --format={json,what_you_want} postmaster_file

To be more precise, storage.sgml documents the format of the PID file
in what seems like the correct order for each item, some of them being
empty depending on the setup.
--
Michael


signature.asc
Description: PGP signature

Re: Fix incorrect start up costs for WindowAgg paths (bug #17862)

[David Rowley]

.On Thu, 13 Apr 2023 at 02:28, Andy Fan  wrote:
> The concept of startup_tuples for a WindowAgg looks good to me, but I
> can't follow up with the below line:
>
> + return clamp_row_est(partition_tuples * DEFAULT_INEQ_SEL);
>
> # select count(*) over() from tenk1 limit 1;
>  count
> ---
>  1  -->  We need to scan all the tuples.
>
> Should we just return clamp_row_est(partition_tuples)?

For the case you've shown, it will.  It's handled by this code:

if (wc->orderClause == NIL)
return clamp_row_est(partition_tuples);

It would take something like the following to hit the code you're
concerned about:

explain select count(*) over(order by unique1 rows between unbounded
preceding and 10*random() following) from tenk1;
 QUERY PLAN

 WindowAgg  (cost=140.23..420.29 rows=1 width=12)
   ->  Index Only Scan using tenk1_unique1 on tenk1
(cost=0.29..270.29 rows=1 width=4)
(2 rows)

You can see the startup cost is about 33% of the total cost for that,
which is from the DEFAULT_INEQ_SEL.  I'm not exactly set on that
having to be DEFAULT_INEQ_SEL, but I'm not really sure what we could
put that's better. I don't really follow why assuming all rows are
required is better.  That'll just mean we favour cheap startup plans
less, but there might be a case where a cheap startup plan is
favourable. I was opting for a happy medium when I thought to use
DEFAULT_INEQ_SEL.

I also see I might need to do a bit more work on this as the following
is not handled correctly:

select count(*) over(rows between unbounded preceding and 10
following) from tenk1;

it's assuming all rows due to lack of ORDER BY, but it seems like it
should be 10 rows due to the 10 FOLLOWING end bound.

David

Re: Add LZ4 compression in pg_dump

[Michael Paquier]

On Tue, Apr 11, 2023 at 08:19:59PM -0500, Justin Pryzby wrote:
> On Wed, Apr 12, 2023 at 10:07:08AM +0900, Michael Paquier wrote:
>> Yes, this comment gives no value as it stands.  I would be tempted to
>> follow the suggestion to group the whole code block in a single ifdef,
>> including the check, and remove this comment.  Like the attached
>> perhaps?
> 
> +1

Let me try this one again, as the previous patch would cause a warning
under --without:-zlib as user_compression_defined would be unused.  We
could do something like the attached instead.  It means doing twice
parse_compress_specification() for the non-zlib path, still we are
already doing so for the zlib path.

If there are other ideas, feel free.
--
Michael
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 967ced4eed..596f64ed2c 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -751,17 +751,21 @@ main(int argc, char **argv)
 
 	/*
 	 * Custom and directory formats are compressed by default with gzip when
-	 * available, not the others.
+	 * available if no compression specification has been specified, not the
+	 * others.
 	 */
-	if ((archiveFormat == archCustom || archiveFormat == archDirectory) &&
-		!user_compression_defined)
+	if (!user_compression_defined)
 	{
+		if (archiveFormat == archCustom || archiveFormat == archDirectory)
+		{
 #ifdef HAVE_LIBZ
-		parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
-	 &compression_spec);
+			parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
+		 &compression_spec);
 #else
-		/* Nothing to do in the default case */
+			parse_compress_specification(PG_COMPRESSION_NONE, NULL,
+		 &compression_spec);
 #endif
+		}
 	}
 
 	/*


signature.asc
Description: PGP signature

Re: [PATCH] Use role name "system_user" instead of "user" for unsafe_tests

[Michael Paquier]

On Wed, Apr 12, 2023 at 03:30:03PM +0300, Aleksander Alekseev wrote:
> Any objections if we remove the tests for "user"?

Based on some rather-recent experience in this area with
COERCE_SQL_SYNTAX, the relationship between the SQL keywords and the
way they can handled internally could be tricky if this area of the
code is touched.  So I would choose to keep these tests, FWIW.
--
Michael


signature.asc
Description: PGP signature

Add ps display while waiting for wal in read_local_xlog_page_guts

[sirisha chamarthi]

Hi,

pg_create_logical_replication_slot can take longer than usual on a standby
when there is no activity on the primary. We don't have enough information
in the pg_stat_activity or process title to debug why this is taking so
long. Attached a small patch to update the process title while waiting for
the wal in read_local_xlog_page_guts. Any thoughts on introducing a new
wait event too?

For example, in my setup, slot creation took 8 minutes 13 seconds. It only
succeeded after I ran select txid_current() on primary.

postgres=# select pg_create_logical_replication_slot('s1','test_decoding');

 pg_create_logical_replication_slot

 (s1,0/C096D10)
(1 row)

Time: 493365.995 ms (08:13.366)

Thanks,
Sirisha


0001-set-ps-display_while-waiting-for-wal.patch
Description: Binary data

Re: Wrong results from Parallel Hash Full Join

[Thomas Munro]

On Thu, Apr 13, 2023 at 9:48 AM Melanie Plageman
 wrote:
> Attached patch includes the fix for ExecParallelHashTableInsert() as
> well as a test. I toyed with adapting one of the existing parallel full
> hash join tests to cover this case, however, I think Richard's repro is
> much more clear. Maybe it is worth throwing in a few updates to the
> tables in the existing queries to provide coverage for the other
> HeapTupleHeaderClearMatch() calls in the code, though.

Oof.  Analysis and code LGTM.

I thought about the way non-parallel HJ also clears the match bits
when re-using the hash table for rescans.  PHJ doesn't keep hash
tables across rescans.  (There's no fundamental reason why it
couldn't, but there was some complication and it seemed absurd to have
NestLoop over Gather over PHJ, forking a new set of workers for every
tuple, so I didn't implement that in the original PHJ.)  But... there
is something a little odd about the code in
ExecHashTableResetMatchFlags(), or the fact that we appear to be
calling it: it's using the unshared union member unconditionally,
which wouldn't actually work for PHJ (there should be a variant of
that function with Parallel in its name if we ever want that to work).
That's not a bug AFAICT, as in fact we don't actually call it--it
should be unreachable because the hash table should be gone when we
rescan--but it's confusing.  I'm wondering if we should put in
something explicit about that, maybe a comment and an assertion in
ExecReScanHashJoin().

+-- Ensure that hash join tuple match bits have been cleared before putting them
+-- into the hashtable.

Could you mention that the match flags steals a bit from the HOT flag,
ie *why* we're testing a join after an update?  And if we're going to
exercise/test that case, should we do the non-parallel version too?

For the commit message, I think it's a good idea to use something like
"Fix ..." for the headline of bug fix commits to make that clearer,
and to add something like "oversight in commit XYZ" in the body, just
to help people connect the dots.  (Yeah, I know I failed to reference
the delinquent commit in the recent assertion-removal commit, my bad.)
 I think "Discussion:" footers are supposed to use
https://postgr.es/m/XXX shortened URLs.

Re: Add LZ4 compression in pg_dump

[Justin Pryzby]

On Thu, Apr 13, 2023 at 07:23:48AM +0900, Michael Paquier wrote:
> On Tue, Apr 11, 2023 at 08:19:59PM -0500, Justin Pryzby wrote:
> > On Wed, Apr 12, 2023 at 10:07:08AM +0900, Michael Paquier wrote:
> >> Yes, this comment gives no value as it stands.  I would be tempted to
> >> follow the suggestion to group the whole code block in a single ifdef,
> >> including the check, and remove this comment.  Like the attached
> >> perhaps?
> > 
> > +1
> 
> Let me try this one again, as the previous patch would cause a warning
> under --without:-zlib as user_compression_defined would be unused.  We
> could do something like the attached instead.  It means doing twice
> parse_compress_specification() for the non-zlib path, still we are
> already doing so for the zlib path.
> 
> If there are other ideas, feel free.

I don't think you need to call parse_compress_specification(NONE).
As you wrote it, if zlib is unavailable, there's no parse(NONE) call,
even for directory and custom formats.  And there's no parse(NONE) call
for plan format when zlib is available.

The old way had preprocessor #if around both the "if" and "else" - is
that what you meant ?

If you don't insist on calling parse(NONE), the only change is to remove
the empty #else, which was my original patch.

"if no compression specification has been specified" is redundant with
"by default", and causes "not the others" to dangle.

If I were to rewrite the comment, it'd say:

+* When gzip is available, custom and directory formats are compressed 
by
+* default

Re: [PATCH] Allow Postgres to pick an unused port to listen

[Yurii Rashkovskii]

Tom, Robert, Greg, Andrew,

On Thu, Apr 13, 2023 at 12:56 AM Tom Lane  wrote:

> Robert Haas  writes:
> > On Wed, Apr 12, 2023 at 1:31 PM Greg Stark  wrote:
> >> I don't object to using the pid file as the mechanism -- but it is a
> >> bit of an awkward UI for shell scripting. I imagine it would be handy
> >> if pg_ctl had an option to just print the port number so you could get
> >> it with a simple port=`pg_ctl -D  status-port`
>
> > That's not a bad idea, and would provide some additional isolation to
> > reduce direct dependency on the PID file format.
>
> Yeah.  My main concern here is with limiting our ability to change
> the pidfile format in future.  If we can keep the dependencies on that
> localized to code we control, it'd be much better.
>
>
Thank you all for the feedback. It's quite useful. I think it is important
to separate this into two concerns:

1. Letting Postgres pick an unused port.
2. Retrieving the port it picked.

If I get this right, there's no significant opposition to (1) as this is
common functionality we're relying on. The most contention is around (2)
because I suggested using postmaster.pid
file, which may be considered private for the most part, at least for the
time being.

With this in mind, I still think that proceeding with (1) is a good idea,
as retrieving the port being listened on is still much easier than
involving a more complex lock file script. For example, on UNIX-like
systems, `lsof` can be typically used to do this:

```
# For IPv4
lsof  -a -w -FPn -p $(head -n 1 postmaster.pid) -i4TCP -sTCP:LISTEN -P -n |
tail -n 1 | awk -F: '{print $NF}'
# For IPv6
lsof  -a -w -FPn -p $(head -n 1postmaster.pid) -i6TCP -sTCP:LISTEN -P -n |
tail -n 1 | awk -F: '{print $NF}'
```

(There are also other tools that can be used to achieve much of the same)

On Windows, this can be done using PowerShell (and perhaps netstat, too):

```
# IPv4
PS> Get-NetTCPConnection -State Listen -OwningProcess (Get-Content
"postmaster.pid" -First 1) | Where-Object { $_.LocalAddress -notmatch ':' }
| Select-Object -ExpandProperty LocalPort
5432
PS> Get-NetTCPConnection -State Listen -OwningProcess (Get-Content
"postmaster.pid" -First 1) | Where-Object { $_.LocalAddress -match ':' } |
Select-Object -ExpandProperty LocalPort
5432
```

The above commands can be worked on to extract multiple ports should that
ever become a feature.

The bottom line is this decouples (1) from (2), and we can resolve them
separately if there's too much (understandable) hesitation to commit to a
particular approach to it (documenting postmaster.pid, changing its format,
amending pg_ctl functionality, etc.) I will be happy to participate in the
discovery and resolution of (2) as well.

This would allow people like myself or Mark (above in the thread) to let
Postgres pick the unused port and extract it using a oneliner for the time
being. When a better approach for server introspection will be agreed on,
we can use that.

I'll be happy to address any [styling or other] issues with the currently
proposed patch.


--
http://omnigres.org
Yurii

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Daniel Gustafsson]

> On 12 Apr 2023, at 23:46, Daniel Gustafsson  wrote:
>> On 12 Apr 2023, at 23:40, Tom Lane  wrote:

>> So this smells to me like a new OpenSSL bug: they should tolerate
>> a missing certs dir like they used to.  Who wants to file it?
> 
> They are specifying that: "A missing default location is still treated as a
> success".  That leaves out the interesting bit of what a success means here,
> and how it should work when verifications are requested.  That being said, the
> same is written in the 1.1.1 manpage.

After a little bit of digging I have a vague idea.

OpenSSL will treat a missing default location as a success simply due to the
fact that it mainly just stores the path, loading of the certs is deferred
until use (which maps well to the error we are seeing).  Patching OpenSSL to
report all errors makes no difference, a missing default is indeed not an error
even with errors turned on.

The change in OpenSSL 3 is the addition of certificate stores via ossl_store
API.  When SSL_CTX_set_default_verify_paths() is called it will in 1.1.1 set
the default (hardcoded) filename and path; in 3 it also sets the default store.
Stores are initialized with a URL, and the default store falls back to using the
default certs dir as the URI as no store is set.

If I patch OpenSSL 3 to skip setting the default store, the tests pass even
with a missing cert directory. This is effectively the 1.1.1 behavior.

The verification error we are hitting is given to us in the verify_cb which
we've short circuited.  The issue we have is that we cannot get PGconn in
verify_cb so logging an error is tricky.

I need to sleep on this before I do some more digging to figure out if OpenSSL
considers this to be the intended behavior, a regression in 3, or if we have a
bug in how we catch verification errors which is exposed by a non-existing
store.  I'll add an open item for this in the morning to track how we'd like to
proceed.

--
Daniel Gustafsson

Re: Fix incorrect start up costs for WindowAgg paths (bug #17862)

[Andy Fan]

On Thu, Apr 13, 2023 at 6:09 AM David Rowley  wrote:

> .On Thu, 13 Apr 2023 at 02:28, Andy Fan  wrote:
> > The concept of startup_tuples for a WindowAgg looks good to me, but I
> > can't follow up with the below line:
> >
> > + return clamp_row_est(partition_tuples * DEFAULT_INEQ_SEL);
> >
> > # select count(*) over() from tenk1 limit 1;
> >  count
> > ---
> >  1  -->  We need to scan all the tuples.
> >
> > Should we just return clamp_row_est(partition_tuples)?
>
> For the case you've shown, it will.  It's handled by this code:
>
> if (wc->orderClause == NIL)
> return clamp_row_est(partition_tuples);
>
> My fault.  I should have real debugging to double check my
understanding, surely I will next time.

It would take something like the following to hit the code you're
> concerned about:
>
> explain select count(*) over(order by unique1 rows between unbounded
> preceding and 10*random() following) from tenk1;
>  QUERY PLAN
>
> 
>  WindowAgg  (cost=140.23..420.29 rows=1 width=12)
>->  Index Only Scan using tenk1_unique1 on tenk1
> (cost=0.29..270.29 rows=1 width=4)
> (2 rows)
>
> You can see the startup cost is about 33% of the total cost for that,
> which is from the DEFAULT_INEQ_SEL.  I'm not exactly set on that
> having to be DEFAULT_INEQ_SEL, but I'm not really sure what we could
> put that's better. I don't really follow why assuming all rows are
> required is better.  That'll just mean we favour cheap startup plans
> less, but there might be a case where a cheap startup plan is
> favourable. I was opting for a happy medium when I thought to use
> DEFAULT_INEQ_SEL.
>

That looks reasonable to me.  My suggestion came from my misreading
before,  It was a bit late in my time zone when writing. Thanks for the
detailed explanation!


>
> I also see I might need to do a bit more work on this as the following
> is not handled correctly:
>
> select count(*) over(rows between unbounded preceding and 10
> following) from tenk1;
>
> it's assuming all rows due to lack of ORDER BY, but it seems like it
> should be 10 rows due to the 10 FOLLOWING end bound.
>
>
True to me.


-- 
Best Regards
Andy Fan

Clean up hba.c of code freeing regexps

[Michael Paquier]

Hi all,

With db4f21e in place, there is no need to worry about explicitely
freeing any regular expressions that may have been compiled when
loading HBA or ident files because MemoryContextDelete() would be
able to take care of that now that these are palloc'd (the definitions
in regcustom.h superseed the ones of regguts.h).

The logic in hba.c that scans all the HBA and ident lines to any
regexps can be simplified a lot.  Most of this code is new in 16~, so
I think that it is worth cleaning up this stuff now rather than wait
for 17 to open for business.  Still, this is optional, and I don't
mind waiting for 17 if the regexp/palloc business proves to be an
issue during beta.

FWIW, one would see leaks in the postmaster process with files like
that on repeated SIGHUPs before db4f21e:
$ cat $PGDATA/pg_hba.conf
local   "/^db\d{2,4}$"  all trust
local   "/^db\d{2,po}$"  all trust
local   "/^db\d{2,4}$"  all trust
$ cat $PGDATA/pg_ident.conf
foo "/^user\d{2,po}$"  bar
foo "/^uesr\d{2,4}$"  bar

With this configuration, there are no leaks on SIGHUPs after db4f21e
as MemoryContextDelete() does all the job.  Please see the attached.

Thoughts or opinions?
--
Michael
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index adedbd3128..d786a01835 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -95,11 +95,6 @@ static MemoryContext parsed_hba_context = NULL;
 /*
  * pre-parsed content of ident mapping file: list of IdentLine structs.
  * parsed_ident_context is the memory context where it lives.
- *
- * NOTE: the IdentLine structs can contain AuthTokens with pre-compiled
- * regular expressions that live outside the memory context. Before
- * destroying or resetting the memory context, they need to be explicitly
- * free'd.
  */
 static List *parsed_ident_lines = NIL;
 static MemoryContext parsed_ident_context = NULL;
@@ -316,30 +311,6 @@ free_auth_token(AuthToken *token)
 		pg_regfree(token->regex);
 }
 
-/*
- * Free a HbaLine.  Its list of AuthTokens for databases and roles may include
- * regular expressions that need to be cleaned up explicitly.
- */
-static void
-free_hba_line(HbaLine *line)
-{
-	ListCell   *cell;
-
-	foreach(cell, line->roles)
-	{
-		AuthToken  *tok = lfirst(cell);
-
-		free_auth_token(tok);
-	}
-
-	foreach(cell, line->databases)
-	{
-		AuthToken  *tok = lfirst(cell);
-
-		free_auth_token(tok);
-	}
-}
-
 /*
  * Copy a AuthToken struct into freshly palloc'd memory.
  */
@@ -2722,30 +2693,14 @@ load_hba(void)
 	if (!ok)
 	{
 		/*
-		 * File contained one or more errors, so bail out, first being careful
-		 * to clean up whatever we allocated.  Most stuff will go away via
-		 * MemoryContextDelete, but we have to clean up regexes explicitly.
+		 * File contained one or more errors, so bail out.  MemoryContextDelete
+		 * is enough to clean up everything, including regexes.
 		 */
-		foreach(line, new_parsed_lines)
-		{
-			HbaLine*newline = (HbaLine *) lfirst(line);
-
-			free_hba_line(newline);
-		}
 		MemoryContextDelete(hbacxt);
 		return false;
 	}
 
 	/* Loaded new file successfully, replace the one we use */
-	if (parsed_hba_lines != NIL)
-	{
-		foreach(line, parsed_hba_lines)
-		{
-			HbaLine*newline = (HbaLine *) lfirst(line);
-
-			free_hba_line(newline);
-		}
-	}
 	if (parsed_hba_context != NULL)
 		MemoryContextDelete(parsed_hba_context);
 	parsed_hba_context = hbacxt;
@@ -3044,8 +2999,7 @@ load_ident(void)
 {
 	FILE	   *file;
 	List	   *ident_lines = NIL;
-	ListCell   *line_cell,
-			   *parsed_line_cell;
+	ListCell   *line_cell;
 	List	   *new_parsed_lines = NIL;
 	bool		ok = true;
 	MemoryContext oldcxt;
@@ -3102,30 +3056,14 @@ load_ident(void)
 	if (!ok)
 	{
 		/*
-		 * File contained one or more errors, so bail out, first being careful
-		 * to clean up whatever we allocated.  Most stuff will go away via
-		 * MemoryContextDelete, but we have to clean up regexes explicitly.
+		 * File contained one or more errors, so bail out.  MemoryContextDelete
+		 * is enough to clean up everything, including regexes.
 		 */
-		foreach(parsed_line_cell, new_parsed_lines)
-		{
-			newline = (IdentLine *) lfirst(parsed_line_cell);
-			free_auth_token(newline->system_user);
-			free_auth_token(newline->pg_user);
-		}
 		MemoryContextDelete(ident_context);
 		return false;
 	}
 
 	/* Loaded new file successfully, replace the one we use */
-	if (parsed_ident_lines != NIL)
-	{
-		foreach(parsed_line_cell, parsed_ident_lines)
-		{
-			newline = (IdentLine *) lfirst(parsed_line_cell);
-			free_auth_token(newline->system_user);
-			free_auth_token(newline->pg_user);
-		}
-	}
 	if (parsed_ident_context != NULL)
 		MemoryContextDelete(parsed_ident_context);
 


signature.asc
Description: PGP signature

Re: Wrong results from Parallel Hash Full Join

[Melanie Plageman]

On Wed, Apr 12, 2023 at 6:50 PM Thomas Munro  wrote:
>
> On Thu, Apr 13, 2023 at 9:48 AM Melanie Plageman
>  wrote:
> > Attached patch includes the fix for ExecParallelHashTableInsert() as
> > well as a test. I toyed with adapting one of the existing parallel full
> > hash join tests to cover this case, however, I think Richard's repro is
> > much more clear. Maybe it is worth throwing in a few updates to the
> > tables in the existing queries to provide coverage for the other
> > HeapTupleHeaderClearMatch() calls in the code, though.
>
> Oof.  Analysis and code LGTM.
>
> I thought about the way non-parallel HJ also clears the match bits
> when re-using the hash table for rescans.  PHJ doesn't keep hash
> tables across rescans.  (There's no fundamental reason why it
> couldn't, but there was some complication and it seemed absurd to have
> NestLoop over Gather over PHJ, forking a new set of workers for every
> tuple, so I didn't implement that in the original PHJ.)  But... there
> is something a little odd about the code in
> ExecHashTableResetMatchFlags(), or the fact that we appear to be
> calling it: it's using the unshared union member unconditionally,
> which wouldn't actually work for PHJ (there should be a variant of
> that function with Parallel in its name if we ever want that to work).
> That's not a bug AFAICT, as in fact we don't actually call it--it
> should be unreachable because the hash table should be gone when we
> rescan--but it's confusing.  I'm wondering if we should put in
> something explicit about that, maybe a comment and an assertion in
> ExecReScanHashJoin().

An assert about it not being a parallel hash join? I support this.

> +-- Ensure that hash join tuple match bits have been cleared before putting 
> them
> +-- into the hashtable.
>
> Could you mention that the match flags steals a bit from the HOT flag,
> ie *why* we're testing a join after an update?

v2 attached has some wordsmithing along these lines.

> And if we're going to
> exercise/test that case, should we do the non-parallel version too?

I've added this. I thought if we were adding the serial case, we might
as well add the multi-batch case as well. However, that proved a bit
more challenging. We can get a HOT tuple in one of the existing tables
with no issues. Doing this and then deleting the reset match bit code
doesn't cause any of the tests to fail, however, because we use this
expression as the join condition when we want to emit NULL-extended
unmatched tuples.

select  count(*) from simple r full outer join simple s on (r.id = 0 - s.id);

I don't think we want to add yet another time-consuming test to this
test file. So, I was trying to decide if it was worth changing these
existing tests so that they would fail when the match bit wasn't reset.
I'm not sure.

> For the commit message, I think it's a good idea to use something like
> "Fix ..." for the headline of bug fix commits to make that clearer,
> and to add something like "oversight in commit XYZ" in the body, just
> to help people connect the dots.  (Yeah, I know I failed to reference
> the delinquent commit in the recent assertion-removal commit, my bad.)

I've made these edits and tried to improve the commit message clarity in
general.

>  I think "Discussion:" footers are supposed to use
> https://postgr.es/m/XXX shortened URLs.

Hmm. Is the problem with mine that I included "flat"? Because I did use
postgr.es/m format. The message id is unfortunately long, but I believe
that is on google and not me.

- Melanie
From ee24229a1eeca67dfd91e162502efe2abfead870 Mon Sep 17 00:00:00 2001
From: Melanie Plageman 
Date: Wed, 12 Apr 2023 17:16:28 -0400
Subject: [PATCH v2] Fix PHJ tuple match bit management

Hash join tuples reuse the HOT status bit to indicate match status
during hash join execution. Correct reuse requires clearing the bit in
all tuples. Serial hash join and parallel multi-batch hash join do so
upon inserting the tuple into the hashtable. Single batch parallel hash
join and batch 0 of unexpected multi-batch hash joins forgot to do this.

It hadn't come up before because hashtable tuple match bits are only
used for right and full outer joins and parallel ROJ and FOJ were
unsupported. 11c2d6fdf5 introduced support for parallel ROJ/FOJ but
neglected to ensure the match bits were reset.

Reported-by: Richard Guo

Discussion: https://postgr.es/m/flat/CAMbWs48Nde1Mv%3DBJv6_vXmRKHMuHZm2Q_g4F6Z3_pn%2B3EV6BGQ%40mail.gmail.com
---
 src/backend/executor/nodeHash.c |  1 +
 src/test/regress/expected/join_hash.out | 37 +
 src/test/regress/sql/join_hash.sql  | 28 ++-
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index a45bd3a315..54c06c5eb3 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -1724,6 +1724,7 @@ retry:
 		/* Store the hash value in the HashJoinTuple header. */
 		hashTuple->h

Re: Add LZ4 compression in pg_dump

[Michael Paquier]

On Wed, Apr 12, 2023 at 05:52:40PM -0500, Justin Pryzby wrote:
> I don't think you need to call parse_compress_specification(NONE).
> As you wrote it, if zlib is unavailable, there's no parse(NONE) call,
> even for directory and custom formats.  And there's no parse(NONE) call
> for plan format when zlib is available.

Yeah, that's not necessary, but I was wondering if it made the code a
bit cleaner, or else the non-zlib path would rely on the default
compression method string.

> The old way had preprocessor #if around both the "if" and "else" - is
> that what you meant?
> 
> If you don't insist on calling parse(NONE), the only change is to remove
> the empty #else, which was my original patch.

Removing the empty else has as problem to create an empty if block,
which could be itself a cause of warnings?

> If I were to rewrite the comment, it'd say:
> 
> +* When gzip is available, custom and directory formats are 
> compressed by
> +* default

Okay.
--
Michael


signature.asc
Description: PGP signature

Re: Add LZ4 compression in pg_dump

[Justin Pryzby]

On Thu, Apr 13, 2023 at 09:37:06AM +0900, Michael Paquier wrote:
> > If you don't insist on calling parse(NONE), the only change is to remove
> > the empty #else, which was my original patch.
> 
> Removing the empty else has as problem to create an empty if block,
> which could be itself a cause of warnings?

I doubt it - in the !HAVE_LIBZ case, it's currently an "if" statement
with nothing but a comment, which isn't a problem.

I think the only issue with an empty "if" is when you have no braces,
like:

if (...)
#if ...
something;
#endif

// problem here //

-- 
Justin

Re: Clean up hba.c of code freeing regexps

[Thomas Munro]

On Thu, Apr 13, 2023 at 12:16 PM Michael Paquier  wrote:
> The logic in hba.c that scans all the HBA and ident lines to any
> regexps can be simplified a lot.  Most of this code is new in 16~, so
> I think that it is worth cleaning up this stuff now rather than wait
> for 17 to open for business.  Still, this is optional, and I don't
> mind waiting for 17 if the regexp/palloc business proves to be an
> issue during beta.

Up to the RMT of course, but it sounds a bit like (1) you potentially
had an open item already until last week (new code in 16 that could
leak regexes), and (2) I missed this when looking for manual memory
management code that could be nuked, probably because it's hiding
behind a few layers of functions call, but there are clearly comments
that are now wrong.  So there are two different ways for a commitfest
lawyer to argue this should be tidied up for 16.

Re: Clean up hba.c of code freeing regexps

[Michael Paquier]

On Thu, Apr 13, 2023 at 12:53:51PM +1200, Thomas Munro wrote:
> Up to the RMT of course, but it sounds a bit like (1) you potentially
> had an open item already until last week (new code in 16 that could
> leak regexes),

Well, not really..  Note that HEAD does not leak regexes, because
changes like 02d3448 have made sure that all these are explicitely
freed when a file is parsed and where there is no need to switch to
the new one because errors were found on the way.  In short, one can
just take the previous conf files I pasted and there will be no leaks
on HEAD in the context of the postmaster, even before bea3d7e.  Sure,
there could be issues if one changed the shape of the code, but all
the existing compiled regexes were covered (this stuff already exists
in ~15 for the regexps of the user name in ident lines).

> and (2) I missed this when looking for manual memory
> management code that could be nuked, probably because it's hiding
> behind a few layers of functions call, but there are clearly comments
> that are now wrong.  So there are two different ways for a commitfest
> lawyer to argue this should be tidied up for 16.

Yes, the comments are incorrect anyway.
--
Michael


signature.asc
Description: PGP signature

Re: Bufmgr possible overflow

[Kyotaro Horiguchi]

Perhaps it's a good idea to seprate the patch for each issue.

At Wed, 12 Apr 2023 09:36:14 -0300, Ranier Vilela  wrote 
in> IMO I think that commit 31966b1
> 
> has an oversight.
> 
> All the logic of the changes are based on the "extend_by" variable, which
> is a uint32, but in some places it is using "int", which can lead to an
> overflow at some point.

int is nowadays is at least 32 bits, so using int in a loop that
iterates up to a uint32 value won't cause an overflow. However, the
fix iteself looks good because it unifies the loop variable types in
similar loops.

On the other hand, I'm not a fan of changing the signature of
smgr_zeroextend to use uint32. I don't think it improves things and
the other reason is that I don't like using unnatural integer types
unnecessarily in API parameter types. ASnyway, the patch causes a type
inconsistency between smgr_zserextend and mdzeroextend.

> I also take the opportunity to correct another oversight, regarding the
> commit dad50f6
> 
> ,
> for possible duplicate assignment.
> GetLocalBufferDescriptor was called twice.
> 
> Taking advantage of this, I promoted a scope reduction for some variables,
> which I thought was opportune.

I like the scope reductions.

Regarding the duplicate assignment to existing_hdr, I prefer assigning
it in the definition line, but I don't have a strong opinion on this
matter.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Greg Stark]

On Wed, Apr 12, 2023, 19:30 Daniel Gustafsson  wrote:
>
>  The issue we have is that we cannot get PGconn in
> verify_cb so logging an error is tricky.


Hm, the man page talks about a "ex_data mechanism" which seems to be
referring to this Rube Goldberg device
https://www.openssl.org/docs/man3.1/man3/SSL_get_ex_data.html

It looks like X509_STORE_CTX_set_app_data() and
X509_STORE_CTX_get_app_data() would be convenience macros to do this.

Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply

[Kyotaro Horiguchi]

At Wed, 12 Apr 2023 23:39:29 +0900, Fujii Masao  
wrote in 
> BTW, you can reproduce the issue even when using a TCP connection
> instead of a Unix domain socket by specifying a very large number
> in the "keepalives" connection parameter for the foreign server.
> Here is an example:
> 
> -
> create server loopback foreign data wrapper postgres_fdw options (host
> '127.0.0.1', port '5432', keepalives '999');
> -

Mmm..

> The reason behind this issue is that PQconnectPoll() parses
> the "keepalives" parameter value by simply using strtol(),
> while PQgetCancel() uses parse_int_param(). To fix this issue,
> it might be better to update PQconnectPoll() so that it uses
> parse_int_param() for parsing the "keepalives" parameter.

Agreed, it seems to be a leftover when we moved to parse_int_param()
in that area.

> > the real issue here is that PGgetCancel is unnecessarily checking its
> > value and failing as a result. Otherwise there would be no room for
> > failure in the call to PQgetCancel() at that point in the example
> > case.
> > PQconnectPoll should remove the ignored parameters at connection or
> > PQgetCancel should ingore the unreferenced (or unchecked)
> > parameters. For example, the below diff takes the latter way and seems
> > working (for at least AF_UNIX connections)
> 
> To clarify, are you suggesting that PQgetCancel() should
> only parse the parameters for TCP connections
> if cancel->raddr.addr.ss_family != AF_UNIX?
> If so, I think that's a good idea.

You're right. I used connip in the diff because I thought it provided
the same condition, but in a simpler way.

> 
> > Of course, it's not great that pgfdw_cancel_query_begin() ignores the
> > result from PQgetCancel(), but I think we don't need another ereport.
> 
> Can you please clarify why you suggest avoiding outputting
> the warning message when PQgetCancel() returns NULL?

No.  I suggested merging the two failures that emit the "same" message
because I believed that they were identical. I had this in my mind:

  calcel = PQgetCancel();
  if (!cancel || PQcancel())
  {
 ereport(); return false;
  }
  PQfreeCancel()

However, I notcied that PQgetCancel() doesn't set errbuf.. So, I'm
fine with your proposal.

> I think it is important to inform the user when an error
> occurs and a cancel request cannot be sent, as this information
> can help them identify the cause of the problem (such as
> setting an overly large value for the keepalives parameter).

Although I view it as an internal error, I agree with emitting some
error messages in that situation.

regrads.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Direct I/O

[Thomas Munro]

Thanks both for looking, and thanks for the explanation Ilmari.
Pushed with your improvements.  The 4 CI systems run the tests
(Windows and Mac by special always-expected-to-work case, Linux and
FreeBSD by successful pre-flight perl test of O_DIRECT), and I also
tested three unusual systems, two that skip for different reasons and
one that will henceforth run this test on the build farm so I wanted
to confirm locally first:

Linux/tmpfs: 1..0 # SKIP pre-flight test if we can open a file with
O_DIRECT failed: Invalid argument
OpenBSD: t/004_io_direct.pl .. skipped: no O_DIRECT
illumos: t/004_io_direct.pl .. ok

(Format different because those last two are autoconf, no meson on my
collection of Vagrant images yet...)