Re: shared-memory based stats collector

[Kyotaro Horiguchi]

At Wed, 2 Mar 2022 18:16:00 -0800, Andres Freund  wrote in 
> Hi,
> 
> On 2021-07-26 18:27:54 -0700, Andres Freund wrote:
> > I had intended to post a rebase by now. But while I did mostly finish
> > that (see [1]) I unfortunately encountered a new issue around
> > partitioned tables, see [2]. Currently I'm hoping for a few thoughts on
> > that thread about the best way to address the issues.
> 
> Now that 
> https://postgr.es/m/20220125063131.4cmvsxbz2tdg6g65%40alap3.anarazel.de
> is resolved, here's a rebased version. With a good bit of further cleanup.
> 
> One "big" thing that I'd like to figure out is a naming policy for the
> different types prefixed with PgStat. We have different groups of types:
> 
> - "pending statistics", that are accumulated but not yet submitted to the
>   shared stats system, like PgStat_TableStatus, PgStat_BackendFunctionEntry
>   etc
> - accumulated statistics like PgStat_StatDBEntry, PgStat_SLRUStats. About 
> half are
>   prefixed with PgStat_Stat, the other just with PgStat_
> - random other types like PgStat_Single_Reset_Type, ...
> 
> To me it's very confusing to have these all in an essentially undistinguishing
> namespace, particularly the top two items.

Profoundly agreed.  It was always a pain in the neck.

> I think we should at least do s/PgStat_Stat/PgStat_/. Perhaps we should use a
> distinct PgStatPending_* for the pending item? I can't quite come up with a
> good name for the "accumulated" ones.

How about naming "pending stats" as just "Stats" and the "acculumated
stats" as "counts" or "counters"?  "Counter" doesn't reflect the
characteristics so exactly but I think the discriminability of the two
is more significant.  Specifically;

- PgStat_TableStatus
+ PgStat_TableStats
- PgStat_BackendFunctionEntry
+ PgStat_FunctionStats

- PgStat_GlobalStats
+ PgStat_GlobalCounts
- PgStat_ArchiverStats
+ PgStat_ArchiverCounts
- PgStat_BgWriterStats
+ PgStat_BgWriterCounts

Moving to shared stats collector turns them into attributed by "Local"
and "Shared". (I don't consider the details at this stage.)

PgStatLocal_TableStats
PgStatLocal_FunctionStats
PgStatLocal_GlobalCounts
PgStatLocal_ArchiverCounts
PgStatLocal_BgWriterCounts

PgStatShared_TableStats
PgStatShared_FunctionStats
PgStatShared_GlobalCounts
PgStatShared_ArchiverCounts
PgStatShared_BgWriterCounts

PgStatLocal_GlobalCounts somewhat looks odd, but doesn't matter much, maybe.

> I'd like that get resolved first because I think that'd allow commiting the
> prepatory split and reordering patches.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Skipping logical replication transactions on subscriber side

[Amit Kapila]

On Tue, Mar 1, 2022 at 8:31 PM Masahiko Sawada  wrote:
>
> I’ve considered a plan for the skipping logical replication
> transaction feature toward PG15. Several ideas and patches have been
> proposed here and another related thread[1][2] for the skipping
> logical replication transaction feature as follows:
>
> A. Change pg_stat_subscription_workers (committed 7a8507329085)
> B. Add origin name and commit-LSN to logical replication worker
> errcontext (proposed[2])
> C. Store error information (e.g., the error message and commit-LSN) to
> the system catalog
> D. Introduce ALTER SUBSCRIPTION SKIP
> E. Record the skipped data somewhere: server logs or a table
>
> Given the remaining time for PG15, it’s unlikely to complete all of
> them for PG15 by the feature freeze. The most realistic plan for PG15
> in my mind is to complete B and D. With these two items, the LSN of
> the error-ed transaction is shown in the server log, and we can ask
> users to check server logs for the LSN and use it with ALTER
> SUBSCRIPTION SKIP command.
>

It makes sense to me to try to finish B and D from the above list for
PG-15. I can review the patch for D in detail if others don't have an
objection to it.

Peter E., others, any opinion on this matter?

> If the community agrees with B+D, we will
> have a user-visible feature for PG15 which can be further
> extended/improved in PG16 by adding C and E.

Agreed.

>
> I've attached an updated patch for D and here is the summary:
>
> * Introduce a new command ALTER SUBSCRIPTION ... SKIP (lsn =
> '0/1234'). The user can get the commit-LSN of the transaction in
> question from the server logs thanks to B[2].
> * The user-specified LSN (say skip-LSN) is stored in the
> pg_subscription catalog.
> * The apply worker skips the whole transaction if the transaction's
> commit-LSN exactly matches to skip-LSN.
> * The skip-LSN has an effect on only the first non-empty transaction
> since the worker started to apply changes. IOW it's cleared after
> either skipping the whole transaction or successfully committing a
> non-empty transaction, preventing the skip-LSN to remain in the
> catalog. Also, since the latter case means that the user set the wrong
> skip-LSN we clear it with a warning.
>

As this will be displayed only in server logs and by background apply
worker, should it be LOG or WARNING?

-- 
With Regards,
Amit Kapila.

Re: libpq compression (part 2)

[Daniil Zakhlystov]

Ok, thanks


> On 3 Mar 2022, at 02:33, Justin Pryzby  wrote:
> 
> If there's no objection, I'd like to move this to the next CF for 
> consideration
> in PG16.
> 
> On Mon, Jan 17, 2022 at 10:39:19PM -0600, Justin Pryzby wrote:
>> On Tue, Jan 18, 2022 at 02:06:32AM +0500, Daniil Zakhlystov wrote:
 => Since March, errmsg doesn't need extra parenthesis around it (e3a87b4).
>> 
>>> I’ve resolved the stuck tests and added zlib support for CI Windows builds 
>>> to patch 0003-*.  Thanks
>>> for the suggestion, all tests seem to be OK now, except the macOS one.  It 
>>> won't schedule in Cirrus
>>> CI for some reason, but I guess it happens because of my GitHub account 
>>> limitation.
>> 
>> I don't know about your github account, but it works for cfbot, which is now
>> green.
>> 
>> Thanks for implementing zlib for windows.  Did you try this with default
>> compressions set to lz4 and zstd ?
>> 
>> The thread from 2019 is very long, and starts off with the guidance that
>> compression had been implemented at the wrong layer.  It looks like this 
>> hasn't
>> changed since then.  secure_read/write are passed as function pointers to the
>> ZPQ interface, which then calls back to them to read and flush its 
>> compression
>> buffers.  As I understand, the suggestion was to leave the socket reads and
>> writes alone.  And then conditionally de/compress buffers after reading /
>> before writing from the socket if compression was negotiated.
>> 
>> It's currently done like this
>> pq_recvbuf() => secure_read() - when compression is disabled 
>> pq_recvbuf() => ZPQ => secure_read() - when compression is enabled 
>> 
>> Dmitri sent a partial, POC patch which changes the de/compression to happen 
>> in
>> secure_read/write, which is changed to call ZPQ:  
>> https://www.postgresql.org/message-id/ca+q6zcuprssnars+fyobsd-f2stk1roa-4sahfofajowlzi...@mail.gmail.com
>> pq_recvbuf() => secure_read() => ZPQ
>> 
>> The same thing is true of the frontend: function pointers to
>> pqsecure_read/write are being passed to zpq_create, and then the ZPQ 
>> interface
>> called instead of the original functions.  Those are the functions which read
>> using SSL, so they should also handle compression.
>> 
>> That's where SSL is handled, and it seems like the right place to handle
>> compression.  Have you evaluated that way to do things ?
>> 
>> Konstantin said he put ZPQ at that layer seems to 1) avoid code duplication
>> between client/server; and, 2) to allow compression to happen before SSL, to
>> allow both (if the admin decides it's okay).  But I don't see why compression
>> can't happen before sending to SSL, or after reading from it?

Re: Support for grabbing multiple consecutive values with nextval()

[Peter Eisentraut]

On 02.03.22 20:12, Jille Timmermans wrote:

On 2022-02-28 11:13, Peter Eisentraut wrote:

On 27.02.22 10:42, Jille Timmermans wrote:
I wanted to be able to allocate a bunch of numbers from a sequence at 
once. Multiple people seem to be struggling with this 
(https://stackoverflow.com/questions/896274/select-multiple-ids-from-a-postgresql-sequence, 
https://www.depesz.com/2008/03/20/getting-multiple-values-from-sequences/). 



I propose to add an extra argument to nextval() that specifies how 
many numbers you want to allocate (default 1).


What is the use of this?

I note that the stackoverflow question wanted to return multiple
sequence values as a result set, whereas your implementation just
skips a number of values and returns the last one.  At least we should
be clear about what we are trying to achieve.
Both would work for me actually. I'm using COPY FROM to insert many rows 
and need to know their ids and COPY FROM doesn't support RETURNING.


I don't understand this use case.  COPY FROM copies from a file.  So you 
want to preallocate the sequence numbers before you copy the new data 
in?  How do you know how many rows are in the file?

Re: Commitfest 2022-03 Patch Triage Part 1a.i

[Yugo NAGATA]

On Tue, 1 Mar 2022 13:39:45 -0500
Greg Stark  wrote:

> As Justin suggested I CC the authors from these patches I'm adding
> them here. Some of the patches have multiple "Authors" listed in the
> commitfest which may just be people who posted updated patches so I
> may have added more people than necessary.
> 
> On Tue, 1 Mar 2022 at 11:16, Greg Stark  wrote:
> >
> > Last November Daniel Gustafsson  did a patch triage. It took him three
> > emails to get through the patches in the commitfest back then. Since
> > then we've had the November and the January commitfests so I was
> > interested to see how many of these patches had advanced
> >
> > I'm only part way through the first email but so far only two patches
> > have changed status -- and both to "Returned with feedback" :(
> >
> > So I'm going to post updates but I'm going to break it up into smaller
> > batches because otherwise it'll take me a month before I post
> > anything.
> >
> >
> >
> > > 1608: schema variables, LET command
> > > ===
> > > After 18 CF's and two very long threads it seems to be nearing completion
> > > judging by Tomas' review.  There is an ask in that review for a second 
> > > pass
> > > over the docs by a native speaker, any takers?
> >
> > Patch has a new name, "session variables, LET command"
> >
> > There's been a *lot* of work on this patch so I'm loath to bump it.
> > The last review was from Julien Rouhaud which had mostly code style
> > comments but it had one particular concern about using xact callback
> > in core and about EOX cleanup.
> >
> > Pavel, do you have a plan to improve this or are you looking for
> > suggestions from someone about how you should solve this problem?
> >
> > > 1741: Index Skip Scan
> > > =
> > > An often requested feature which has proven hard to reach consensus on an
> > > implementation for.  The thread(s) have stalled since May, is there any 
> > > hope of
> > > taking this further?  Where do we go from here with this patch?
> >
> > "Often requested indeed! I would love to be able to stop explaining to
> > people that Postgres can't handle this case well.
> >
> > It seems there are multiple patch variants around and suggestions from
> > Heikki and Peter G about fundamental interface choices. It would be
> > nice to have a good summary from someone who is involved about what's
> > actually left unresolved.
> >
> >
> > > 1712: Remove self join on a unique column
> > > =
> > > This has moved from "don't write bad queries" to "maybe we should do 
> > > something
> > > about this".  It seems like there is concensus that it can be worth 
> > > paying the
> > > added planner cost for this, especially if gated by a GUC to keep it out 
> > > of
> > > installations where it doesn't apply.  The regression on large join 
> > > queries
> > > hasn't been benchmarked it seems (or I missed it), but the patch seems to 
> > > have
> > > matured and be quite ready.  Any takers on getting it across the finish 
> > > line?
> >
> > There hasn't been any review since the v29 patch was posted in July.
> > That said, to my eye it seemed like pretty basic functionality errors
> > were being corrected quite late. All the bugs got patch updates
> > quickly but does this have enough tests to be sure it's working right?
> >
> > The only real objection was about whether the planning time justified
> > the gains since the gains are small. But I think they were resolved by
> > making the optimization optional. Do we have consensus that that
> > resolved the issue or do we still need benchmarks showing the planning
> > time hit is reasonable?
> >
> > > 2161: standby recovery fails when re-replaying due to missing directory 
> > > which
> > > was removed in previous replay.
> > > =
> > > Tom and Robert seem to be in agreement that parts of this patchset are 
> > > good,
> > > and that some parts are not.  The thread has stalled and the patch no 
> > > longer
> > > apply, so unless someone feels like picking up the good pieces this seems 
> > > like
> > > a contender to close for now.
> >
> > Tom's feedback seems to have been acted on last November. And the last
> > update in January was that it was passing CI now. Is this ready to
> > commit now?
> >
> >
> > > 2138: Incremental Materialized View Maintenance
> > > ===
> > > The size of the
> > > patchset and the length of the thread make it hard to gauge just far away 
> > > it
> > > is, maybe the author or a reviewer can summarize the current state and 
> > > outline
> > > what is left for it to be committable.
> >
> > There is an updated patch set as of February but I have the same
> > difficulty wrapping my head around the amount of info here.
> >
> > Is this one really likely to be commitable in 15? If not I think we
> > should move this to 16 now and conc

RE: Logical replication timeout problem

[kuroda.hay...@fujitsu.com]

Dear Wang,

> Attach the new patch. [suggestion by Kuroda-San]
> 1. Fix the typo.
> 2. Improve comment style.
> 3. Fix missing consideration.
> 4. Add comments to clarifies above functions and function calls.

Thank you for updating the patch! I confirmed they were fixed.

```
case REORDER_BUFFER_CHANGE_INVALIDATION:
-   /* Execute the invalidation messages 
locally */
-   ReorderBufferExecuteInvalidations(
-   
  change->data.inval.ninvalidations,
-   
  change->data.inval.invalidations);
-   break;
+   {
+   LogicalDecodingContext *ctx = 
rb->private_data;
+
+   Assert(!ctx->fast_forward);
+
+   /* Set output state. */
+   ctx->accept_writes = true;
+   ctx->write_xid = txn->xid;
+   ctx->write_location = 
change->lsn;
```

Some codes were added in ReorderBufferProcessTXN() for treating DDL, 




I'm also happy if you give the version number :-).


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

> -Original Message-
> From: Wang, Wei/王 威 
> Sent: Wednesday, March 2, 2022 11:06 AM
> To: Kuroda, Hayato/黒田 隼人 
> Cc: Fabrice Chapuis ; Simon Riggs
> ; Petr Jelinek
> ; Tang, Haiying/唐 海英
> ; Amit Kapila ;
> PostgreSQL Hackers ; Ajin Cherian
> 
> Subject: RE: Logical replication timeout problem
> 
> On Mon, Feb 28, 2022 at 6:58 PM Kuroda, Hayato/黒田 隼人
>  wrote:
> > Dear Wang,
> >
> > > Attached a new patch that addresses following improvements I have got
> > > so far as
> > > comments:
> > > 1. Consider other changes that need to be skipped(truncate, DDL and
> > > function calls pg_logical_emit_message). [suggestion by Ajin, Amit]
> > > (Add a new function SendKeepaliveIfNecessary for trying to send
> > > keepalive
> > > message.)
> > > 2. Set the threshold conservatively to a static value of
> > > 1.[suggestion by Amit, Kuroda-San] 3. Reset sendTime in function
> > > WalSndUpdateProgress when send_keep_alive is false. [suggestion by
> > > Amit]
> >
> > Thank you for giving a good patch! I'll check more detail later, but it can 
> > be
> > applied my codes and passed check world.
> > I put some minor comments:
> Thanks for your comments.
> 
> > ```
> > + * Try to send keepalive message
> > ```
> >
> > Maybe missing "a"?
> Fixed. Add missing "a".
> 
> > ```
> > +   /*
> > +   * After continuously skipping SKIPPED_CHANGES_THRESHOLD
> changes, try
> > to send a
> > +   * keepalive message.
> > +   */
> > ```
> >
> > This comments does not follow preferred style:
> > https://www.postgresql.org/docs/devel/source-format.html
> Fixed. Correct wrong comment style.
> 
> > ```
> > @@ -683,12 +683,12 @@ OutputPluginWrite(struct LogicalDecodingContext
> *ctx,
> > bool last_write)
> >   * Update progress tracking (if supported).
> >   */
> >  void
> > -OutputPluginUpdateProgress(struct LogicalDecodingContext *ctx)
> > +OutputPluginUpdateProgress(struct LogicalDecodingContext *ctx, bool
> > +send_keep_alive)
> > ```
> >
> > This function is no longer doing just tracking.
> > Could you update the code comment above?
> Fixed. Update the comment above function OutputPluginUpdateProgress.
> 
> > ```
> > if (!is_publishable_relation(relation))
> > return;
> > ```
> >
> > I'm not sure but it seems that the function exits immediately if relation 
> > is a
> > sequence, view, temporary table and so on. Is it OK? Does it never happen?
> I did some checks to confirm this. After my confirmation, there are several
> situations that can cause a timeout. For example, if I insert many date into
> table sql_features in a long transaction, subscriber-side will time out.
> Although I think users should not modify these tables arbitrarily, it could
> happen. To be conservative, I think this use case should be addressed as well.
> Fixed. Invoke function SendKeepaliveIfNecessary before return.
> 
> > ```
> > +   SendKeepaliveIfNecessary(ctx, false);
> > ```
> >
> > I think a comment is needed above which clarifies sending a keepalive
> message.
> Fixed. Before invoking function SendKeepaliveIfNecessary, add the
> corresponding
> comment.
> 
> Attach the new patch. [suggestion by Kuroda-San]
> 1. Fix the typo.
> 2. Improve comment style.
> 3. Fix missing consideration.
> 4. Add comments to clarifies above functions and function calls.
> 
> Regards,
> Wang wei

Re: Support for grabbing multiple consecutive values with nextval()

[Jille Timmermans]

On 2022-03-03 10:10, Peter Eisentraut wrote:

On 02.03.22 20:12, Jille Timmermans wrote:

On 2022-02-28 11:13, Peter Eisentraut wrote:

On 27.02.22 10:42, Jille Timmermans wrote:
I wanted to be able to allocate a bunch of numbers from a sequence 
at once. Multiple people seem to be struggling with this 
(https://stackoverflow.com/questions/896274/select-multiple-ids-from-a-postgresql-sequence, 
https://www.depesz.com/2008/03/20/getting-multiple-values-from-sequences/). 
I propose to add an extra argument to nextval() that specifies how 
many numbers you want to allocate (default 1).


What is the use of this?

I note that the stackoverflow question wanted to return multiple
sequence values as a result set, whereas your implementation just
skips a number of values and returns the last one.  At least we 
should

be clear about what we are trying to achieve.
Both would work for me actually. I'm using COPY FROM to insert many 
rows and need to know their ids and COPY FROM doesn't support 
RETURNING.


I don't understand this use case.  COPY FROM copies from a file.  So
you want to preallocate the sequence numbers before you copy the new
data in?

Yes


How do you know how many rows are in the file?
I'm using https://pkg.go.dev/github.com/jackc/pgx/v4#Conn.CopyFrom, 
which uses the COPY FROM protocol but doesn't actually have to originate 
from a file.

Re: Proposal: Support custom authentication methods using hooks

[Peter Eisentraut]

On 02.03.22 21:26, Stephen Frost wrote:

Part of the point, for my part anyway, of dropping support for plaintext
transmission would be to remove support for that from libpq, otherwise a
compromised server could still potentially convince a client to provide
a plaintext password be sent to it.


I think there should be a generalized feature for client-side selecting 
or filtering of authentication methods.  As long as there exists more 
than one method, there will be tradeoffs and users might want to avoid 
one or the other.  I don't think removing a method outright is the right 
solution for that.

Re: Proposal: Support custom authentication methods using hooks

[Peter Eisentraut]

On 02.03.22 16:45, Jonathan S. Katz wrote:
By that argument, we should have kept "password" (plain) as an 
authentication method.


For comparison, the time between adding md5 and removing password was 16 
years.  It has been 5 years since scram was added.

Re: Proposal: Support custom authentication methods using hooks

[Peter Eisentraut]

On 02.03.22 15:16, Jonathan S. Katz wrote:
What are the reasons they are still purposely using it? The ones I have 
seen/heard are:


- Using an older driver
- On a pre-v10 PG
- Unaware of SCRAM


Another reason is that SCRAM presents subtle operational issues in 
distributed systems.  As someone who is involved with products such as 
pgbouncer and bdr, I am aware that there are still unresolved problems 
and ongoing research in that area.  Maybe they can all be solved 
eventually, even if it is concluding "you can't do that anymore" in 
certain cases, but it's not all solved yet, and falling back to the 
best-method-before-this-one is a useful workaround.


I'm thinking there might be room for an authentication method between 
plain and scram that is less complicated and allows distributed systems 
to be set up more easily.  I don't know what that would be, but I don't 
think we should prohibit the consideration of "anything less than SCRAM".


I notice that a lot of internet services are promoting "application 
passwords" nowadays.  I don't know the implementation details of that, 
but it appears that the overall idea is to have instead of one 
high-value password have many frequently generated medium-value 
passwords.  We also have a recent proposal to store multiple passwords 
per user.  (Obviously that could apply to SCRAM and not-SCRAM equally.) 
That's the kind of direction I would like to explore.

Re: Make mesage at end-of-recovery less scary.

[Ashutosh Sharma]

On Wed, Mar 2, 2022 at 7:47 AM Kyotaro Horiguchi
 wrote:
>
> At Sat, 19 Feb 2022 09:31:33 +0530, Ashutosh Sharma  
> wrote in
> > The changes looks good. thanks.!
>
> Thanks!
>
> Some recent core change changed WAL insertion speed during the TAP
> test and revealed one forgotton case of EndOfWAL.  When a record
> header flows into the next page, XLogReadRecord does separate check
> from ValidXLogRecordHeader by itself.
>

The new changes made in the patch look good. Thanks to the recent
changes to speed WAL insertion that have helped us catch this bug.

One small comment:

record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ);
-   total_len = record->xl_tot_len;

Do you think we need to change the position of the comments written
for above code that says:

/*
 * Read the record length.
 *
...
...

--
With Regards,
Ashutosh Sharma.

Re: Proposal: Support custom authentication methods using hooks

[Peter Eisentraut]

On 02.03.22 21:49, samay sharma wrote:
I think we are discussing two topics in this thread which in my opinion 
are orthogonal.


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


I don't think people are necessarily opposed to that.

At the moment, it is not possible to judge whether the hook interface 
you have chosen is appropriate.


I suggest you actually implement the Azure provider, then make the hook 
interface, and then show us both and we can see what to do with it.


One thing that has been requested, and I would support that, is that a 
plugged-in authentication method should look like a built-in one.  So 
for example it should be able to register a real name, instead of 
"custom".  I think a fair bit of refactoring work might be appropriate 
in order to make the authentication code more modular.

Re: Mingw task for Cirrus CI

[Melih Mutlu]

Hi Andres,


> This presumably is due to using mingw's python rather than python.org's
> python. Seems like a reasonable thing to support for the mingw build.
>
> Melih, you could try to build against the python.org python (i installed
> in
> the CI container).
>

I tried to use the installed python from python.org in the container.
The problem with this is that "LIBDIR" and "LDLIBRARY" configs of python
for windows from python.org are empty. Therefore python_libdir or other
related variables in configure file are not set correctly.


Seems we're doing something wrong and end up with a 4 byte off_t, whereas
> python ends up with an 8byte one. We probably need to fix that. But it's
> not
> the cause of this problem.
>
>
> You could take out -s from the make flags and see whether plpython3.dll is
> being built and installed, and where to.
>

Here is a run without -s: https://cirrus-ci.com/task/4569363104661504
I couldn't get what's wrong from these logs to be honest. But I see that
plpython3.dll exists under src/pl/plpython after build when I run these
steps locally.

Best,
Melih

Re: row filtering for logical replication

[Amit Kapila]

On Thu, Mar 3, 2022 at 11:18 AM shiy.f...@fujitsu.com
 wrote:
>
> On Thu, Mar 3, 2022 10:40 AM Amit Kapila  wrote:
> >
> > On Wed, Mar 2, 2022 at 5:42 PM Euler Taveira  wrote:
> > >
> > > On Wed, Mar 2, 2022, at 8:45 AM, Tomas Vondra wrote:
> > >
> > > While working on the column filtering patch, which touches about the
> > > same places, I noticed two minor gaps in testing:
> > >
> > > 1) The regression tests do perform multiple ALTER PUBLICATION commands,
> > > tweaking the row filter. But there are no checks the row filter was
> > > actually modified / stored in the catalog. It might be just thrown away
> > > and no one would notice.
> > >
> > > The test that row filter was modified is available in a previous section. 
> > > The
> > > one that you modified (0001) is testing the supported objects.
> > >
> >
> > Right. But if Tomas thinks it is good to add for these ones as well
> > then I don't mind.
> >
> > > 153 ALTER PUBLICATION testpub5 ADD TABLE testpub_rf_tbl3 WHERE (e > 1000
> > AND e < 2000);
> > > 154 \dRp+ testpub5
> > > 155 ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl2;
> > > 156 \dRp+ testpub5
> > > 157 -- remove testpub_rf_tbl1 and add testpub_rf_tbl3 again (another WHERE
> > expression)
> > > 158 ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl3 WHERE (e > 300
> > AND e < 500);
> > > 159 \dRp+ testpub5
> > >
> > > IIRC this test was written before adding the row filter information into 
> > > the
> > > psql. We could add \d+ testpub_rf_tbl3 before and after the modification.
> > >
> >
> >
> > Agreed. We can use \d instead of \d+ as row filter is available with \d.
> >
> > > 2) There are no pg_dump tests.
> > >
> > > WFM.
> > >
> >
> > This is a miss. I feel we can add a few more.
> >
>
> Agree that we can add some tests, attach the patch which fixes these two 
> points.
>

LGTM. I'll push this tomorrow unless Tomas or Euler feels otherwise.

-- 
With Regards,
Amit Kapila.

Re: Add 64-bit XIDs into PostgreSQL 15

[Pavel Borisov]

>
> > The patch doesn't apply - I suppose the patch is relative a forked
> postgres
>
> No, the authors just used a little outdated `master` branch. I
> successfully applied it against 31d8d474 and then rebased to the
> latest master (62ce0c75). The new version is attached.

Not 100% sure if my rebase is correct since I didn't invest too much
> time into reviewing the code. But at least it passes `make
> installcheck` locally. Let's see what cfbot will tell us.
>
Thank you very much! We'll do the same rebase and check this soon. Let's
use v10 now.


> > I encourage trying to break down the patch into smaller incrementally
> useful
> > pieces. E.g. making all the SLRUs 64bit would be a substantial and
> > independently committable piece.
>
> Completely agree. And the changes like:
>
> +#if 0 /* XXX remove unit tests */
>
> ... suggest that the patch is pretty raw in its current state.
>
> Pavel, Maxim, don't you mind me splitting the patchset, or would you
> like to do it yourself and/or maybe include more changes? I don't know
> how actively you are working on this.
>
I don't mind and appreciate you joining this. If you split this we'll just
make the next versions based on it. Of course, there is much to do and we
work on this patch, including pg_upgrade test fail reported by Justin,
which we haven't had time to concentrate on before. We try to do changes in
small batches so I consider we can manage parallel changes. At least I read
this thread very often and can answer soon, even if our new versions of
patches are not ready.

Again I consider the work you propose useful and big thanks to you,
Alexander!

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Add 64-bit XIDs into PostgreSQL 15

[Pavel Borisov]

BTW messages with patches in this thread are always invoke manual spam
moderation and we need to wait for ~3 hours before the message with patch
becomes visible in the hackers thread. Now when I've already answered
Alexander's letter with v10 patch the very message (and a patch) I've
answered is still not visible in the thread and to CFbot.

Can something be done in hackers' moderation engine to make new versions
patches become visible hassle-free?

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Problem with moderation of messages with patched attached.

[Pavel Borisov]

Hi, hackers!

Around 2 months ago I've noticed a problem that messages containing patches
in the thread [1] were always processed with manual moderation. They appear
in hackers' thread hours after posting None of them are from new CF members
and personally, I don't see a reason for such inconvenience. The problem
still exists as of today.

Can someone make changes in a moderation engine to make it more liberal and
convenient for authors?

[1]
https://www.postgresql.org/message-id/flat/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Problem with moderation of messages with patched attached.

[Maxim Orlov]

On Thu, Mar 3, 2022 at 3:31 PM Pavel Borisov  wrote:

> Hi, hackers!
>
> Around 2 months ago I've noticed a problem that messages containing
> patches in the thread [1] were always processed with manual moderation.
> They appear in hackers' thread hours after posting None of them are from
> new CF members and personally, I don't see a reason for such inconvenience.
> The problem still exists as of today.
>
> Can someone make changes in a moderation engine to make it more liberal
> and convenient for authors?
>
> [1]
> https://www.postgresql.org/message-id/flat/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com
>
> --
> Best regards,
> Pavel Borisov
>
> Postgres Professional: http://postgrespro.com 
>

Confirm

-- 
Best regards,
Maxim Orlov.

Re: Commitfest 2022-03 Patch Triage Part 1a.i

[Daniel Gustafsson]

> On 3 Mar 2022, at 10:11, Yugo NAGATA  wrote:

> I think this patch set needs more reviews to be commitable in 15, so I
> returned the target version to blank. I'll change it to 16 later.

I've added 16 as a target version in the CF app now.

--
Daniel Gustafsson   https://vmware.com/

Re: Mingw task for Cirrus CI

[Andrew Dunstan]

On 3/3/22 05:16, Melih Mutlu wrote:
> Hi Andres, 
>  
>
> This presumably is due to using mingw's python rather than
> python.org 's
> python. Seems like a reasonable thing to support for the mingw build.
>
> Melih, you could try to build against the python.org
>  python (i installed in
> the CI container).
>
>
> I tried to use the installed python from python.org
>  in the container. 
> The problem with this is that "LIBDIR" and "LDLIBRARY" configs of
> python for windows from python.org  are empty.
> Therefore python_libdir or other related variables in configure file
> are not set correctly.



Yeah, here's what it has:


# python -c "import sysconfig; import pprint; pp =
pprint.PrettyPrinter(); pp.pprint(sysconfig.get_config_vars())"
{'BINDIR': 'C:\\prog\\python310',
 'BINLIBDEST': 'C:\\prog\\python310\\Lib',
 'EXE': '.exe',
 'EXT_SUFFIX': '.cp310-win_amd64.pyd',
 'INCLUDEPY': 'C:\\prog\\python310\\Include',
 'LIBDEST': 'C:\\prog\\python310\\Lib',
 'SO': '.cp310-win_amd64.pyd',
 'TZPATH': '',
 'VERSION': '310',
 'abiflags': '',
 'base': 'C:\\prog\\python310',
 'exec_prefix': 'C:\\prog\\python310',
 'installed_base': 'C:\\prog\\python310',
 'installed_platbase': 'C:\\prog\\python310',
 'platbase': 'C:\\prog\\python310',
 'platlibdir': 'lib',
 'prefix': 'C:\\prog\\python310',
 'projectbase': 'C:\\prog\\python310',
 'py_version': '3.10.2',
 'py_version_nodot': '310',
 'py_version_nodot_plat': '310',
 'py_version_short': '3.10',
 'srcdir': 'C:\\prog\\python310',
 'userbase': 'C:\\Users\\Administrator\\AppData\\Roaming\\Python'}


The DLL lives in the BINDIR, so maybe I guess we should search there if
we can't get the other things.


cheers


andrew

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

Re: Problem with moderation of messages with patched attached.

[Aleksander Alekseev]

Hi hackers,

> Confirm

Here are my two cents.

My last email to pgsql-jobs@ was moderated in a similar fashion. To my
knowledge that mailing list is not pre-moderated. So it may have the same
problem, and not only with patches. (We use regular Google Workspace.)

The pgsql-hackers@ thread under question seems to have two mailing list
addresses in cc:. Maybe this is the reason [1]:

> Cross-posted emails will be moderated and therefore will also take longer
to reach the subscribers if approved.

Although it's strange that only emails with attachments seem to be affected.

[1]: https://www.postgresql.org/list/

-- 
Best regards,
Aleksander Alekseev

Re: Add the replication origin name and commit-LSN to logical replication worker errcontext

[Masahiko Sawada]

On Thu, Mar 3, 2022 at 3:37 PM Amit Kapila  wrote:
>
> On Wed, Mar 2, 2022 at 1:05 PM Masahiko Sawada  wrote:
> >
> > On Wed, Mar 2, 2022 at 4:14 PM Amit Kapila  wrote:
> >
> > I've attached updated patches.
> >
>
> The first patch LGTM. Some comments on the second patch:
>
> 1.
> @@ -3499,6 +3503,17 @@ ApplyWorkerMain(Datum main_arg)
>   myslotname = MemoryContextStrdup(ApplyContext, syncslotname);
>
>   pfree(syncslotname);
> +
> + /*
> + * Allocate the origin name in long-lived context for error context
> + * message
> + */
> + ReplicationOriginNameForTablesync(MySubscription->oid,
> +   MyLogicalRepWorker->relid,
> +   originname,
> +   sizeof(originname));
> + apply_error_callback_arg.origin_name = MemoryContextStrdup(ApplyContext,
> +originname);
>
> Can we assign this in LogicalRepSyncTableStart() where we already
> forming the origin name? That will avoid this extra call to
> ReplicationOriginNameForTablesync.

Yes, but it requires to expose either apply_error_callback_arg or the
tablesync's origin name since the tablesync worker sets its origin
name in tablesync.c. I think it's better to avoid exposing them.

>
> 2.
> @@ -3657,30 +3679,37 @@ apply_error_callback(void *arg)
>   errcontext("processing remote data during \"%s\"",
>  logicalrep_message_type(errarg->command));
>   else
> - errcontext("processing remote data during \"%s\" in transaction %u",
> + errcontext("processing remote data during \"%s\" in transaction %u
> committed at LSN %X/%X from replication origin \"%s\"",
>  logicalrep_message_type(errarg->command),
> -errarg->remote_xid);
> +errarg->remote_xid,
> +LSN_FORMAT_ARGS(errarg->commit_lsn),
> +errarg->origin_name);
>
> Won't we set the origin name before the command? If so, it can be used
> in the first message as well and we can change the condition in the
> beginning such that if the origin or command is not set then we can
> return without adding additional context information.

Good point.

>
> Isn't this error message used for rollback prepared failure as well?
> If so, do we need to say "... finished at LSN ..." instead of "...
> committed at LSN ..."?

Right. Or can we just remove "committed" since the current message is
"transaction %u at %s"? That is , just replace the timestamp with LSN.

>
> The other point about this message is that saying " from
> replication origin ..." sounds more like remote information similar to
> publication but the origin is of the local node. How about slightly
> changing it to "processing remote data for replication origin \"%s\"
> during \"%s\" in transaction ..."?

Okay, so the modified message would be like:

"processing remote data for replication origin \"%s\" during \"%s\"
for replication target relation \"%s.%s\" column \"%s\" in transaction
%u finished at LSN %X/%X"

>
> 3.
> +
> +   The LSN of the transaction that contains the change violating the
> constraint and
> +   the replication origin name can be found from those outputs (LSN
> 0/14C0378 and
> +   replication origin pg_16395 in the above case).
> The transaction
> +   can be skipped by calling the  linkend="pg-replication-origin-advance">
> pg_replication_origin_advance() function with
> -   a node_name corresponding to the subscription name,
> -   and a position.  The current position of origins can be seen in the
> -   
> +   the node_name and the next LSN of the commit LSN
> +   (i.e., 0/14C0379) from those outputs.
>
> After node_name, can we specify origin_name similar to what we do for
> LSN as that will make things more clear? Also, shall we mention that
> users need to disable subscriptions to perform replication origin
> advance?

Agreed.

>
> I think for prepared transactions, the user might need to use it twice
> because after skipping prepared xact, it will get an error again
> during the processing of commit prepared (no such prepare exists).

Good point.

>  I
> thought of mentioning it but felt that might lead to specifying too
> many details which can confuse users as well. What do you think?

Given that this method of using pg_replication_origin_advance() is
normally not a preferable way, I think we might not need to mention
it. Also, it needs twice for one transaction but the steps are the
same.

>
> 4. There are places in the patch like apply_handle_stream_start()
> which sets commit_lsn in callback arg as InvalidXLogRecPtr but the
> callback function seems to be assuming that it is always a valid
> value. Shouldn't we need to avoid appending LSN for such cases?

Agreed. Will fix it in the next version patch.

I'm updating the patches and will submit them.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set

[Andrew Dunstan]

On 3/3/22 00:03, Michael Paquier wrote:
>>> +if (   (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
>>> +   || (!defined($ENV{olddump}) && defined($ENV{oldinstall})))
>> Odd indentation. Spaces between parens?
> Well, perltidy tells me that this is right.
>
>

Yeah, I haven't found a way to make it stop doing that :-(


cheers


andrew

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

Re: Problem with moderation of messages with patched attached.

[Dave Page]

Hi

On Thu, 3 Mar 2022 at 12:31, Pavel Borisov  wrote:

> Hi, hackers!
>
> Around 2 months ago I've noticed a problem that messages containing
> patches in the thread [1] were always processed with manual moderation.
> They appear in hackers' thread hours after posting None of them are from
> new CF members and personally, I don't see a reason for such inconvenience.
> The problem still exists as of today.
>
> Can someone make changes in a moderation engine to make it more liberal
> and convenient for authors?
>
> [1]
> https://www.postgresql.org/message-id/flat/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com
>

Here's the moderation reason for that message:

Message to list pgsql-hackers held for moderation due to 'Size 1MB (1061796
bytes) is larger than threshold 1000KB (1024000 bytes)', notice queued for
2 moderators

-- 
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

Re: Problem with moderation of messages with patched attached.

[Pavel Borisov]

>
> Message to list pgsql-hackers held for moderation due to 'Size 1MB
> (1061796 bytes) is larger than threshold 1000KB (1024000 bytes)', notice
> queued for 2 moderators
>
Could you make this limit 2MB at least for authorized commitfest members?
Thanks!

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Problem with moderation of messages with patched attached.

[Aleksander Alekseev]

Hi Dave,

> Message to list pgsql-hackers held for moderation due to 'Size 1MB (1061796 
> bytes) is larger than threshold 1000KB (1024000 bytes)', notice queued for 2 
> moderators

Thanks! Does anyone know if cfbot understands .patch.gz and/or .tgz ?

-- 
Best regards,
Aleksander Alekseev

Re: Problem with moderation of messages with patched attached.

[Dave Page]

On Thu, 3 Mar 2022 at 13:22, Pavel Borisov  wrote:

> Message to list pgsql-hackers held for moderation due to 'Size 1MB
>> (1061796 bytes) is larger than threshold 1000KB (1024000 bytes)', notice
>> queued for 2 moderators
>>
> Could you make this limit 2MB at least for authorized commitfest members?
> Thanks!
>

The mail system doesn't have the capability to apply different moderation
rules for people in that way I'm afraid.

-- 
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

Re: Problem with moderation of messages with patched attached.

[Pavel Borisov]

>
> The mail system doesn't have the capability to apply different moderation
> rules for people in that way I'm afraid.
>
Maybe then 2MB for everyone? Otherwise it's not so convenient. Lead to
answers before the questions in the thread [1], seems weird.

[1]
https://www.postgresql.org/message-id/flat/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com



-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Problem with moderation of messages with patched attached.

[Julien Rouhaud]

On Thu, Mar 03, 2022 at 04:24:03PM +0300, Aleksander Alekseev wrote:
> 
> Thanks! Does anyone know if cfbot understands .patch.gz and/or .tgz ?

There's a FAQ link on the cfbot main page that answers this kind of questions.

Re: Add 64-bit XIDs into PostgreSQL 15

[Alexander Korotkov]

Hi Pavel!

On Thu, Mar 3, 2022 at 2:35 PM Pavel Borisov  wrote:
> BTW messages with patches in this thread are always invoke manual spam 
> moderation and we need to wait for ~3 hours before the message with patch 
> becomes visible in the hackers thread. Now when I've already answered 
> Alexander's letter with v10 patch the very message (and a patch) I've 
> answered is still not visible in the thread and to CFbot.
>
> Can something be done in hackers' moderation engine to make new versions 
> patches become visible hassle-free?

Is your email address subscribed to the pgsql-hackers mailing list?
AFAIK, moderation is only applied for non-subscribers.

--
Regards,
Alexander Korotkov

Re: Problem with moderation of messages with patched attached.

[Pavel Borisov]

>
> There's a FAQ link on the cfbot main page that answers this kind of
> questions.
>
Good to know! I'll try [.gz] next time then.

Thanks!

Re: Problem with moderation of messages with patched attached.

[Dave Page]

On Thu, 3 Mar 2022 at 13:28, Pavel Borisov  wrote:

> The mail system doesn't have the capability to apply different moderation
>> rules for people in that way I'm afraid.
>>
> Maybe then 2MB for everyone? Otherwise it's not so convenient. Lead to
> answers before the questions in the thread [1], seems weird.
>

Then someone will complain if their patch is 2.1MB! How often are messages
legitimately over 1MB anyway, even with a patch? I don't usually moderate
-hackers, so I don't know if this is a common thing or not.

I'll ping a message across to the sysadmin team anyway; I can't just change
that setting without buy-in from the rest of the team.

-- 
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

Re: Add 64-bit XIDs into PostgreSQL 15

[Pavel Borisov]

>
> > BTW messages with patches in this thread are always invoke manual spam
> moderation and we need to wait for ~3 hours before the message with patch
> becomes visible in the hackers thread. Now when I've already answered
> Alexander's letter with v10 patch the very message (and a patch) I've
> answered is still not visible in the thread and to CFbot.
> >
> > Can something be done in hackers' moderation engine to make new versions
> patches become visible hassle-free?
>
> Is your email address subscribed to the pgsql-hackers mailing list?
> AFAIK, moderation is only applied for non-subscribers.
>
Hi, Alexander!

Yes, it is in the list. The problem is that patch is over 1Mb. So it
strictly goes through moderation. And this is unchanged for 2 months
already.
I was advised to use .gz, which I will do next time.

I've requested increasing threshold to 2 MB [1]

[1]
https://www.postgresql.org/message-id/flat/CALT9ZEGbAR84q_emsf1TUMPqXT%3Dc8CxN16g-HQCxgkLzekM%2BQg%40mail.gmail.com
--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Problem with moderation of messages with patched attached.

[Aleksander Alekseev]

Hi Dave,

> Then someone will complain if their patch is 2.1MB! How often are messages 
> legitimately over 1MB anyway, even with a patch? I don't usually moderate 
> -hackers, so I don't know if this is a common thing or not.
>
> I'll ping a message across to the sysadmin team anyway; I can't just change 
> that setting without buy-in from the rest of the team.

IMO, current limits are OK. The actual problem is that when the
message gets into moderation, the notice to the author doesn't contain
the reason:

> Your message to pgsql-hackers with subject
> "Re: Add 64-bit XIDs into PostgreSQL 15"
> has been held for moderation.
>
> It will be delivered to the list recipients as soon as it has been
> approved by a moderator.
>
> If you wish to cancel the message without delivery, please click
> this link: 

Any chance we could include the reason in the message? I foresee that
otherwise such kinds of questions will be asked over and over again.

--
Best regards,
Aleksander Alekseev

Re: Problem with moderation of messages with patched attached.

[Julien Rouhaud]

On Thu, Mar 03, 2022 at 01:37:35PM +, Dave Page wrote:
>
> Then someone will complain if their patch is 2.1MB! How often are messages
> legitimately over 1MB anyway, even with a patch? I don't usually moderate
> -hackers, so I don't know if this is a common thing or not.

It's not common, most people send compressed versions.

Also, gigantic patchsets tend to be hard to maintain and rot pretty fast, so
authors also sometimes maintain a branch on some external repository and just
send newer versions on the ML infrequently.

Re: Problem with moderation of messages with patched attached.

[Pavel Borisov]

>
> On Thu, 3 Mar 2022 at 13:28, Pavel Borisov  wrote:
>
>> The mail system doesn't have the capability to apply different moderation
>>> rules for people in that way I'm afraid.
>>>
>> Maybe then 2MB for everyone? Otherwise it's not so convenient. Lead to
>> answers before the questions in the thread [1], seems weird.
>>
>
> Then someone will complain if their patch is 2.1MB! How often are messages
> legitimately over 1MB anyway, even with a patch? I don't usually moderate
> -hackers, so I don't know if this is a common thing or not.
>
Hi, Dave!
Authors in the mentioned thread [1] bump into this issue while posting all
11 versions of a patchset. It is little bit more than 1MB. We can try to
use .gz and if this doesn't work we report it again.

I'll ping a message across to the sysadmin team anyway; I can't just change
> that setting without buy-in from the rest of the team.
>
Thanks! Maybe this will solve the issue.

[1]
https://www.postgresql.org/message-id/flat/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Problem with moderation of messages with patched attached.

[Aleksander Alekseev]

Hi again,

> Any chance we could include the reason in the message? I foresee that
> otherwise such kinds of questions will be asked over and over again.

A link to the list of common reasons should work too.

-- 
Best regards,
Aleksander Alekseev

Re: Allow parallel plan for referential integrity checks?

[Frédéric Yhuel]

Hello, sorry for the late reply.

On 2/14/22 15:33, Robert Haas wrote:

On Mon, Feb 7, 2022 at 5:26 AM Frédéric Yhuel  wrote:

I noticed that referential integrity checks aren't currently
parallelized. Is it on purpose?


It's not 100% clear to me that it is safe. But on the other hand, it's
also not 100% clear to me that it is unsafe.

Generally, I only added CURSOR_OPT_PARALLEL_OK in places where I was
confident that nothing bad would happen, and this wasn't one of those
places. It's something of a nested-query environment -- your criterion
#6. How do we know that the surrounding query isn't already parallel?
Perhaps because it must be DML,


Did you mean DDL?


but I think it must be more important
to support parallel DML than to support this.




I'm not sure what the right thing to do here is, but I think it would
be good if your patch included a test case.



Would that be a regression test? (in src/test/regress ?)

If yes, what should I check? Is it good enough to load auto_explain and 
check that the query triggered by a foreign key addition is parallelized?


Best regards,
Frédéric

Re: Add id's to various elements in protocol.sgml

[Brar Piening]

On 02.03.2022 at 18:54, Chapman Flack wrote:

Perhaps there are a bunch of variablelists where no one cares about
linking to any of the entries.

So maybe a useful non-terminating message to add eventually would
be one that identifies any varlistentry lacking an id, with a
variablelist where at least one other entry has an id.


That sounds like areasonable approach for now.

Is there anybody objecting to pursue this? Do you need more examples how
it would look like?

It would be a bit hurtful to generate a patch that manually adds ~600
ids just to have it rejected as unwanted.

Re: pg_walinspect - a new extension to get raw WAL data and WAL stats

[Nitin Jadhav]

Here are a few comments.

1)
> > > ==
> > >
> > > +--
> > > +-- pg_get_wal_records_info_till_end_of_wal()
> > > +--
> > > +CREATE FUNCTION pg_get_wal_records_info_till_end_of_wal(IN start_lsn 
> > > pg_lsn,
> > > +OUT lsn pg_lsn,
> > > +OUT prev_lsn pg_lsn,
> > > +OUT xid xid,
> > >
> > > Why is this function required? Is pg_get_wal_records_info() alone not
> > > enough? I think it is. See if we can make end_lsn optional in
> > > pg_get_wal_records_info() and lets just have it alone. I think it can
> > > do the job of pg_get_wal_records_info_till_end_of_wal function.
>
> I rather agree to Ashutosh. This feature can be covered by
> pg_get_wal_records(start_lsn, NULL, false).
> I don't think that validations are worth doing at least for the first
> two, as far as that value has a special meaning. I see it useful if
> pg_get_wal_records_info() means dump the all available records for the
> moment, or records of the last segment, page or something.
> *I* also think the function is not needed, as explained above.  Why do
> we need that function while we know how far we can read WAL records
> *before* calling the function?

I agree with this. The function prototype comes first and the
validation can be done accordingly. I feel we can even support
'pg_get_wal_record_info' with the same name. All 3 function's
objectives are the same. So it is better to use the same name
(pg_wal_record_info) with different prototypes.

2) The function 'pg_get_first_valid_wal_record_lsn' looks redundant as
we are getting the same information from the function
'pg_get_first_and_last_valid_wal_record_lsn'. With this function, we
can easily fetch the first lsn. So IMO we should remove
'pg_get_first_valid_wal_record_lsn'.

3) The word 'get' should be removed from the function name(*_get_*) as
all the functions of the extension are used only to get the
information. It will also sync with xlogfuncs's naming conventions
like pg_current_wal_lsn, pg_walfile_name, etc.

4) The function names can be modified with lesser words by retaining
the existing meaning.
:s/pg_get_raw_wal_record/pg_wal_raw_record
:s/pg_get_first_valid_wal_record_lsn/pg_wal_first_lsn
:s/pg_get_first_and_last_valid_wal_record_lsn/pg_wal_first_and_last_lsn
:s/pg_get_wal_record_info/pg_wal_record_info
:s/pg_get_wal_stats/pg_wal_stats

5) Even 'pg_get_wal_stats' and 'pg_get_wal_stats_till_end_of_wal' can
be clubbed as one function.

The above comments are trying to simplify the extension APIs and to
make it easy for the user to understand and use it.

Thanks & Regards,
Nitin Jadhav




On Thu, Mar 3, 2022 at 8:20 AM Kyotaro Horiguchi
 wrote:
>
> Hi.
>
> +#ifdef FRONTEND
> +/*
> + * Functions that are currently not needed in the backend, but are better
> + * implemented inside xlogreader.c because of the internal facilities 
> available
> + * here.
> + */
> +
>  #endif /* FRONTEND */
>
> Why didn't you remove the emptied #ifdef section?
>
> +int
> +read_local_xlog_page_2(XLogReaderState *state, XLogRecPtr targetPagePtr,
> +  int reqLen, XLogRecPtr 
> targetRecPtr, char *cur_page)
>
> The difference with the original function is this function has one
> additional if-block amid. I think we can insert the code directly in
> the original function.
>
> +   /*
> +* We are trying to read future WAL. Let's not wait 
> for WAL to be
> +* available if indicated.
> +*/
> +   if (state->private_data != NULL)
>
> However, in the first place it seems to me there's not need for the
> function to take care of no_wait affairs.
>
> If, for expample, pg_get_wal_record_info() with no_wait = true, it is
> enough that the function identifies the bleeding edge of WAL then loop
> until the LSN.  So I think no need for the new function, nor for any
> modification on the origical function.
>
> The changes will reduce the footprint of the patch largely, I think.
>
> At Wed, 2 Mar 2022 22:37:43 +0530, Bharath Rupireddy 
>  wrote in
> > On Wed, Mar 2, 2022 at 8:12 PM Ashutosh Sharma  
> > wrote:
> > >
> > > Some review comments on v5 patch (v5-0001-pg_walinspect.patch)
> >
> > Thanks for reviewing.
> >
> > > +--
> > > +-- pg_get_wal_records_info()
> > > +--
> > > +CREATE FUNCTION pg_get_wal_records_info(IN start_lsn pg_lsn,
> > > +IN end_lsn pg_lsn,
> > > +IN wait_for_wal boolean DEFAULT false,
> > > +OUT lsn pg_lsn,
> > >
> > > What does the wait_for_wal flag mean here when one has already
> > > specified the start and end lsn? AFAIU, If a user has specified a
> > > start and stop LSN, it means that the user knows the extent to which
> > > he/she wants to display the WAL records in which case we need to stop
> > > once the end lsn has reached . So what is the meaning of wait_for_wal
> > > flag? Does it look sensible to have the wait_for_wal flag here? To me
> > > it doesn't.
> >
> > U

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

[Ashutosh Sharma]

Here are some of my review comments on the latest patch:

+ 
+  
+   type text
+  
+  
+   Type of checkpoint. See .
+  
+ 
+
+ 
+  
+   kind text
+  
+  
+   Kind of checkpoint. See .
+  
+ 

This looks a bit confusing. Two columns, one with the name "checkpoint
types" and another "checkpoint kinds". You can probably rename
checkpoint-kinds to checkpoint-flags and let the checkpoint-types be
as-it-is.

==

+  
pg_stat_progress_checkpointpg_stat_progress_checkpoint
+  One row only, showing the progress of the checkpoint.

Let's make this message consistent with the already existing message
for pg_stat_wal_receiver. See description for pg_stat_wal_receiver
view in "Dynamic Statistics Views" table.

==

[local]:5432 ashu@postgres=# select * from pg_stat_progress_checkpoint;
-[ RECORD 1 ]-+-
pid   | 22043
type  | checkpoint
kind  | immediate force wait requested time

I think the output in the kind column can be displayed as {immediate,
force, wait, requested, time}. By the way these are all checkpoint
flags so it is better to display it as checkpoint flags instead of
checkpoint kind as mentioned in one of my previous comments.

==

[local]:5432 ashu@postgres=# select * from pg_stat_progress_checkpoint;
-[ RECORD 1 ]-+-
pid   | 22043
type  | checkpoint
kind  | immediate force wait requested time
start_lsn | 0/14C60F8
start_time| 2022-03-03 18:59:56.018662+05:30
phase | performing two phase checkpoint


This is the output I see when the checkpointer process has come out of
the two phase checkpoint and is currently writing checkpoint xlog
records and doing other stuff like updating control files etc. Is this
okay?

==

The output of log_checkpoint shows the number of buffers written is 3
whereas the output of pg_stat_progress_checkpoint shows it as 0. See
below:

2022-03-03 20:04:45.643 IST [22043] LOG:  checkpoint complete: wrote 3
buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
write=24.652 s, sync=104.256 s, total=3889.625 s; sync files=2,
longest=0.011 s, average=0.008 s; distance=0 kB, estimate=0 kB

--

[local]:5432 ashu@postgres=# select * from pg_stat_progress_checkpoint;
-[ RECORD 1 ]-+-
pid   | 22043
type  | checkpoint
kind  | immediate force wait requested time
start_lsn | 0/14C60F8
start_time| 2022-03-03 18:59:56.018662+05:30
phase | finalizing
buffers_total | 0
buffers_processed | 0
buffers_written   | 0

Any idea why this mismatch?

==

I think we can add a couple of more information to this view -
start_time for buffer write operation and start_time for buffer sync
operation. These are two very time consuming tasks in a checkpoint and
people would find it useful to know how much time is being taken by
the checkpoint in I/O operation phase. thoughts?

--
With Regards,
Ashutosh Sharma.

On Wed, Mar 2, 2022 at 4:45 PM Nitin Jadhav
 wrote:
>
> Thanks for reviewing.
>
> > > > I suggested upthread to store the starting timeline instead.  This way 
> > > > you can
> > > > deduce whether it's a restartpoint or a checkpoint, but you can also 
> > > > deduce
> > > > other information, like what was the starting WAL.
> > >
> > > I don't understand why we need the timeline here to just determine
> > > whether it's a restartpoint or checkpoint.
> >
> > I'm not saying it's necessary, I'm saying that for the same space usage we 
> > can
> > store something a bit more useful.  If no one cares about having the 
> > starting
> > timeline available for no extra cost then sure, let's just store the kind
> > directly.
>
> Fixed.
>
> > 2) Can't we just have these checks inside CASE-WHEN-THEN-ELSE blocks
> > directly instead of new function pg_stat_get_progress_checkpoint_kind?
> > + snprintf(ckpt_kind, MAXPGPATH, "%s%s%s%s%s%s%s%s%s",
> > + (flags == 0) ? "unknown" : "",
> > + (flags & CHECKPOINT_IS_SHUTDOWN) ? "shutdown " : "",
> > + (flags & CHECKPOINT_END_OF_RECOVERY) ? "end-of-recovery " : "",
> > + (flags & CHECKPOINT_IMMEDIATE) ? "immediate " : "",
> > + (flags & CHECKPOINT_FORCE) ? "force " : "",
> > + (flags & CHECKPOINT_WAIT) ? "wait " : "",
> > + (flags & CHECKPOINT_CAUSE_XLOG) ? "wal " : "",
> > + (flags & CHECKPOINT_CAUSE_TIME) ? "time " : "",
> > + (flags & CHECKPOINT_FLUSH_ALL) ? "flush-all" : "");
>
> Fixed.
> ---
>
> > 5) Do we need a special phase for this checkpoint operation? I'm not
> > sure in which cases it will take a long time, but it looks like
> > there's a wait loop here.
> > vxids = GetVirtualXIDsDelayingChkpt(&nvxids);
> > if (nvxids > 0)
> > {
> > do
> > {
> > pg_usleep(1L); /* wait for 10 msec */
> > } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids));
> > }
>
> Yes. It is better to add a separate phase here.
> ---
>

Re: pg_walinspect - a new extension to get raw WAL data and WAL stats

[Ashutosh Sharma]

I think we should also see if we can allow end users to input timeline
information with the pg_walinspect functions. This will help the end
users to get information about WAL records from previous timeline
which can be helpful in case of restored servers.

--
With Regards,
Ashutosh Sharma.

On Thu, Mar 3, 2022 at 8:20 AM Kyotaro Horiguchi
 wrote:
>
> Hi.
>
> +#ifdef FRONTEND
> +/*
> + * Functions that are currently not needed in the backend, but are better
> + * implemented inside xlogreader.c because of the internal facilities 
> available
> + * here.
> + */
> +
>  #endif /* FRONTEND */
>
> Why didn't you remove the emptied #ifdef section?
>
> +int
> +read_local_xlog_page_2(XLogReaderState *state, XLogRecPtr targetPagePtr,
> +  int reqLen, XLogRecPtr 
> targetRecPtr, char *cur_page)
>
> The difference with the original function is this function has one
> additional if-block amid. I think we can insert the code directly in
> the original function.
>
> +   /*
> +* We are trying to read future WAL. Let's not wait 
> for WAL to be
> +* available if indicated.
> +*/
> +   if (state->private_data != NULL)
>
> However, in the first place it seems to me there's not need for the
> function to take care of no_wait affairs.
>
> If, for expample, pg_get_wal_record_info() with no_wait = true, it is
> enough that the function identifies the bleeding edge of WAL then loop
> until the LSN.  So I think no need for the new function, nor for any
> modification on the origical function.
>
> The changes will reduce the footprint of the patch largely, I think.
>
> At Wed, 2 Mar 2022 22:37:43 +0530, Bharath Rupireddy 
>  wrote in
> > On Wed, Mar 2, 2022 at 8:12 PM Ashutosh Sharma  
> > wrote:
> > >
> > > Some review comments on v5 patch (v5-0001-pg_walinspect.patch)
> >
> > Thanks for reviewing.
> >
> > > +--
> > > +-- pg_get_wal_records_info()
> > > +--
> > > +CREATE FUNCTION pg_get_wal_records_info(IN start_lsn pg_lsn,
> > > +IN end_lsn pg_lsn,
> > > +IN wait_for_wal boolean DEFAULT false,
> > > +OUT lsn pg_lsn,
> > >
> > > What does the wait_for_wal flag mean here when one has already
> > > specified the start and end lsn? AFAIU, If a user has specified a
> > > start and stop LSN, it means that the user knows the extent to which
> > > he/she wants to display the WAL records in which case we need to stop
> > > once the end lsn has reached . So what is the meaning of wait_for_wal
> > > flag? Does it look sensible to have the wait_for_wal flag here? To me
> > > it doesn't.
> >
> > Users can always specify a future end_lsn and set wait_for_wal to
> > true, then the pg_get_wal_records_info/pg_get_wal_stats functions can
> > wait for the WAL. IMO, this is useful. If you remember you were okay
> > with wait/nowait versions for some of the functions upthread [1]. I'm
> > not going to retain this behaviour for both
> > pg_get_wal_records_info/pg_get_wal_stats as it is similar to
> > pg_waldump's --follow option.
>
> I agree to this for now. However, I prefer that NULL or invalid
> end_lsn is equivalent to pg_current_wal_lsn().
>
> > > ==
> > >
> > > +--
> > > +-- pg_get_wal_records_info_till_end_of_wal()
> > > +--
> > > +CREATE FUNCTION pg_get_wal_records_info_till_end_of_wal(IN start_lsn 
> > > pg_lsn,
> > > +OUT lsn pg_lsn,
> > > +OUT prev_lsn pg_lsn,
> > > +OUT xid xid,
> > >
> > > Why is this function required? Is pg_get_wal_records_info() alone not
> > > enough? I think it is. See if we can make end_lsn optional in
> > > pg_get_wal_records_info() and lets just have it alone. I think it can
> > > do the job of pg_get_wal_records_info_till_end_of_wal function.
>
> I rather agree to Ashutosh. This feature can be covered by
> pg_get_wal_records(start_lsn, NULL, false).
>
> > > ==
> > >
> > > +--
> > > +-- pg_get_wal_stats_till_end_of_wal()
> > > +--
> > > +CREATE FUNCTION pg_get_wal_stats_till_end_of_wal(IN start_lsn pg_lsn,
> > > +OUT resource_manager text,
> > > +OUT count int8,
> > >
> > > Above comment applies to this function as well. Isn't pg_get_wal_stats() 
> > > enough?
> >
> > I'm doing the following input validations for these functions to not
> > cause any issues with invalid LSN. If I were to have the default value
> > for end_lsn as 0/0, I can't perform input validations right? That is
> > the reason I'm having separate functions {pg_get_wal_records_info,
> > pg_get_wal_stats}_till_end_of_wal() versions.
> >
> > /* Validate input. */
> > if (XLogRecPtrIsInvalid(start_lsn))
> > ereport(ERROR,
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> >  errmsg("invalid WAL record start LSN")));
> >
> > if (XLogRecPtrIsInvalid(end_lsn))
> > ereport(ERROR,
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> >  errmsg

Re: Problem with moderation of messages with patched attached.

[Tom Lane]

Pavel Borisov  writes:
>> The mail system doesn't have the capability to apply different moderation
>> rules for people in that way I'm afraid.

> Maybe then 2MB for everyone?

Maybe your patch needs to be split up?  You're going to have a hard time
finding people who want to review or commit such large chunks.

regards, tom lane

casting operand to proper type in BlockIdGetBlockNumber

[Zhihong Yu]

Hi,
In test output, I saw:

src/backend/utils/adt/tid.c:112:16: runtime error: left shift of 65535 by
16 places cannot be represented in type 'int'

I think this was due to the left shift in BlockIdGetBlockNumber not
properly casting its operand.

Please see the proposed change in patch.

Thanks


get-block-number.patch
Description: Binary data

Re: Problem with moderation of messages with patched attached.

[Julien Rouhaud]

On Thu, Mar 03, 2022 at 10:17:06AM -0500, Tom Lane wrote:
> Pavel Borisov  writes:
> >> The mail system doesn't have the capability to apply different moderation
> >> rules for people in that way I'm afraid.
> 
> > Maybe then 2MB for everyone?
> 
> Maybe your patch needs to be split up?  You're going to have a hard time
> finding people who want to review or commit such large chunks.

I think it's the total attachment size, not a single file.  So while splitting
up the patchset even more would still be a good idea, compressing the files
before sending them to hundred of people would be an even better one.

Re: casting operand to proper type in BlockIdGetBlockNumber

[Tom Lane]

Zhihong Yu  writes:
> In test output, I saw:
> src/backend/utils/adt/tid.c:112:16: runtime error: left shift of 65535 by
> 16 places cannot be represented in type 'int'

What compiler is that?

regards, tom lane

Re: casting operand to proper type in BlockIdGetBlockNumber

[Zhihong Yu]

On Thu, Mar 3, 2022 at 7:44 AM Tom Lane  wrote:

> Zhihong Yu  writes:
> > In test output, I saw:
> > src/backend/utils/adt/tid.c:112:16: runtime error: left shift of 65535 by
> > 16 places cannot be represented in type 'int'
>
> What compiler is that?
>
> regards, tom lane
>
Hi,
Jenkins build is alma8-clang12-asan

So it is clang12 on alma.

Cheers

Re: Proposal: Support custom authentication methods using hooks

[Joshua Brindle]

On Thu, Mar 3, 2022 at 4:45 AM Peter Eisentraut
 wrote:
>
> On 02.03.22 16:45, Jonathan S. Katz wrote:
> > By that argument, we should have kept "password" (plain) as an
> > authentication method.
>
> For comparison, the time between adding md5 and removing password was 16
> years.  It has been 5 years since scram was added.

It's been 7 years since this thread:
https://www.postgresql.org/message-id/54dbcbcf.9000...@vmware.com

As Jonathan and Stephen and others have said, anyone who wishes to
continue using MD5 or other plaintext methods can keep doing that for
5 more years with a supported version of PG. There is no excuse to
leave well known, flawed mechanisms in PG16.

Re: [Proposal] Global temporary tables

[Robert Haas]

On Wed, Mar 2, 2022 at 4:18 PM Andres Freund  wrote:
> I think there's just no way that it can be merged with anything close to the
> current design - it's unmaintainable. The need for the feature doesn't change
> that.

I don't know whether the design is right or wrong, but I agree that a
bad design isn't OK just because we need the feature. I'm not entirely
convinced that the change to _bt_getrootheight() is a red flag,
although I agree that there is a need to explain and justify why
similar changes aren't needed in other places. But I think overall
this patch is just too big and too unpolished to be seriously
considered. It clearly needs to be broken down into incremental
patches that are not just separated by topic but potentially
independently committable, with proposed commit messages for each.

And, like, there's a long history on this thread of people pointing
out particular crash bugs and particular problems with code comments
or whatever and I guess those are getting fixed as they are reported,
but I do not have the feeling that the overall code quality is
terribly high, because people just keep finding more stuff. Like look
at this:

+ uint8 flags = 0;
+
+ /* return 0 if feature is disabled */
+ if (max_active_gtt <= 0)
+ return InvalidTransactionId;
+
+ /* Disable in standby node */
+ if (RecoveryInProgress())
+ return InvalidTransactionId;
+
+ flags |= PROC_IS_AUTOVACUUM;
+ flags |= PROC_IN_LOGICAL_DECODING;
+
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+ arrayP = procArray;
+ for (index = 0; index < arrayP->numProcs; index++)
+ {
+ int pgprocno = arrayP->pgprocnos[index];
+ PGPROC*proc = &allProcs[pgprocno];
+ uint8 statusFlags = ProcGlobal->statusFlags[index];
+ TransactionId gtt_frozenxid = InvalidTransactionId;
+
+ if (statusFlags & flags)
+ continue;

This looks like code someone wrote, modified multiple times as they
found problems, and never cleaned up. 'flags' gets set to 0, and then
unconditionally gets two bits xor'd in, and then we test it against
statusFlags. Probably there shouldn't be a local variable at all, and
if there is, the value should be set properly from the start instead
of constructed incrementally as we go along. And there should be
comments. Why is it OK to return InvalidTransactionId in standby mode?
Why is it OK to pass that flags value? And, if we look at this
function a little further down, is it really OK to hold ProcArrayLock
across an operation that could perform multiple memory allocation
operations? I bet it's not, unless calls are very infrequent in
practice.

I'm not asking for this particular part of the code to be cleaned up.
I'm asking for the whole patch to be cleaned up. Like, nobody who is a
committer is going to have enough time to go through the patch
function by function and point out issues on this level of detail in
every place where they occur. Worse, discussing all of those issues is
just a distraction from the real task of figuring out whether the
design needs adjustment. Because the patch is one massive code drop,
and with not-really-that-clean code and not-that-great comments, it's
almost impossible to review. I don't plan to try unless the quality
improves a lot. I'm not saying it's the worst code ever written, but I
think it's kind of at a level of "well, it seems to work for me," and
the standard around here is higher than that. It's not the job of the
community or of individual committers to prove that problems exist in
this patch and therefore it shouldn't be committed. It's the job of
the author to prove that there aren't and it should be. And I don't
think we're close to that at all.

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

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Dilip Kumar]

On Tue, Mar 1, 2022 at 5:15 PM Dilip Kumar  wrote:

> On Tue, Feb 22, 2022 at 8:27 PM Ashutosh Sharma 
> wrote:
>
>> I'm not sure about the current status, but found it while playing
>> around with the latest changes a bit, so thought of sharing it here.
>>
>> +  
>> +   strategy
>> +   
>> +
>> + This is used for copying the database directory.  Currently, we
>> have
>> + two strategies the WAL_LOG_BLOCK and the
>>
>> Isn't it wal_log instead of wal_log_block?
>>
>> I think when users input wrong strategy with createdb command, we
>> should provide a hint message showing allowed values for strategy
>> types along with an error message. This will be helpful for the users.
>>
>
> I will fix these two comments while posting the next version.
>
>

The new version of the patch fixes these 2 comments pointed by Ashutosh and
also splits the GetRelListFromPage() function as suggested by Robert and
uses the latest snapshot for scanning the pg_class instead of active
snapshot as pointed out by Robert.  I haven't yet added the test case to
create a database using this new strategy option.  So if we are okay with
these two options FILE_COPY and WAL_LOG then I will add test cases for the
same.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From ba8e991e9b24ed8986d4504c44ce85f78cc4a598 Mon Sep 17 00:00:00 2001
From: dilipkumar 
Date: Mon, 4 Oct 2021 13:50:44 +0530
Subject: [PATCH v10 2/6] Extend relmap interfaces

Support new interfaces in relmapper, 1) Support copying the
relmap file from one database path to the other database path.
2) And another interface for getting filenode from oid.  We already
have RelationMapOidToFilenode for the same purpose but that assumes
we are connected to the database for which we want to get the mapping.
So this new interface will do the same but instead, it will get the
mapping for the input database.

These interfaces are required for next patch, for supporting the
wal logged created database.
---
 src/backend/utils/cache/relmapper.c | 123 +++-
 src/include/utils/relmapper.h   |   6 +-
 2 files changed, 113 insertions(+), 16 deletions(-)

diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 56495f0..86a85c8 100644
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -141,7 +141,7 @@ static void read_relmap_file(char *mapfilename, RelMapFile *map,
 static void write_relmap_file_internal(char *mapfilename, RelMapFile *newmap,
 	   bool write_wal, bool send_sinval,
 	   bool preserve_files, Oid dbid, Oid tsid,
-	   const char *dbpath);
+	   const char *dbpath, bool create);
 static void load_relmap_file(bool shared, bool lock_held);
 static void write_relmap_file(bool shared, RelMapFile *newmap,
 			  bool write_wal, bool send_sinval, bool preserve_files,
@@ -256,6 +256,37 @@ RelationMapFilenodeToOid(Oid filenode, bool shared)
 }
 
 /*
+ * RelationMapOidToFilenodeForDatabase
+ *
+ * Same as RelationMapOidToFilenode, but instead of reading the mapping from
+ * the database we are connected to it will read the mapping from the input
+ * database.
+ */
+Oid
+RelationMapOidToFilenodeForDatabase(char *dbpath, Oid relationId)
+{
+	RelMapFile	map;
+	int			i;
+	char		mapfilename[MAXPGPATH];
+
+	/* Relmap file path for the given dbpath. */
+	snprintf(mapfilename, sizeof(mapfilename), "%s/%s",
+			 dbpath, RELMAPPER_FILENAME);
+
+	/* Read the relmap file from the source database. */
+	read_relmap_file(mapfilename, &map, false);
+
+	/* Iterate over the relmap entries to find the input relation oid. */
+	for (i = 0; i < map.num_mappings; i++)
+	{
+		if (relationId == map.mappings[i].mapoid)
+			return map.mappings[i].mapfilenode;
+	}
+
+	return InvalidOid;
+}
+
+/*
  * RelationMapUpdateMap
  *
  * Install a new relfilenode mapping for the specified relation.
@@ -693,7 +724,43 @@ RestoreRelationMap(char *startAddress)
 }
 
 /*
- * read_relmap_file -- read data from given mapfilename file.
+ * CopyRelationMap
+ *
+ * Copy relmapfile from source db path to the destination db path and WAL log
+ * the operation.  This function is only called during the create database, so
+ * the destination database is not yet visible to anyone else, thus we don't
+ * need to acquire the relmap lock while updating the destination relmap.
+ */
+void
+CopyRelationMap(Oid dbid, Oid tsid, char *srcdbpath, char *dstdbpath)
+{
+	RelMapFile map;
+	char mapfilename[MAXPGPATH];
+
+	/* Relmap file path of the source database. */
+	snprintf(mapfilename, sizeof(mapfilename), "%s/%s",
+			 srcdbpath, RELMAPPER_FILENAME);
+
+	/* Read the relmap file from the source database. */
+	read_relmap_file(mapfilename, &map, false);
+
+	/* Relmap file path of the destination database. */
+	snprintf(mapfilename, sizeof(mapfilename), "%s/%s",
+			 dstdbpath, RELMAPPER_FILENAME);
+
+	/*
+	 * Write map contents into the destination database's relm

Re: casting operand to proper type in BlockIdGetBlockNumber

[Tom Lane]

Zhihong Yu  writes:
> On Thu, Mar 3, 2022 at 7:44 AM Tom Lane  wrote:
>> Zhihong Yu  writes:
>>> In test output, I saw:
>>> src/backend/utils/adt/tid.c:112:16: runtime error: left shift of 65535 by
>>> 16 places cannot be represented in type 'int'

> Jenkins build is alma8-clang12-asan

Oh, I misread this as a compile-time warning, but it must be from ASAN.
Was the test case one of your own, or just our normal regression tests?

(I think the code is indeed incorrect, but I'm wondering why this hasn't
been reported before.  It's been like that for a long time.)

regards, tom lane

Re: [PATCH v2] use has_privs_for_role for predefined roles

[Joshua Brindle]

On Thu, Feb 10, 2022 at 2:37 PM Joe Conway  wrote:
>
> On 2/10/22 14:28, Nathan Bossart wrote:
> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
> >> On 2/9/22 13:13, Nathan Bossart wrote:
> >>> I do wonder if users find the differences between predefined roles and 
> >>> role
> >>> attributes confusing.  INHERIT doesn't govern role attributes, but it will
> >>> govern predefined roles when this patch is applied.  Maybe the role
> >>> attribute system should eventually be deprecated in favor of using
> >>> predefined roles for everything.  Or perhaps the predefined roles should 
> >>> be
> >>> converted to role attributes.
> >>
> >> Yep, I was suggesting that the latter would have been preferable to me 
> >> while
> >> Robert seemed to prefer the former. Honestly I could be happy with either 
> >> of
> >> those solutions, but as I alluded to that is probably a discussion for the
> >> next development cycle since I don't see us doing that big a change in this
> >> one.
> >
> > I agree.  I still think Joshua's proposed patch is a worthwhile improvement
> > for v15.
>
> +1
>
> I am planning to get into it in detail this weekend. So far I have
> really just ensured it merges cleanly and passes make world.
>

Rebased patch to apply to master attached.


v5-0001-use-has_privs_for_roles-for-predefined-role-checks.patch
Description: Binary data

Re: casting operand to proper type in BlockIdGetBlockNumber

[Zhihong Yu]

On Thu, Mar 3, 2022 at 8:24 AM Tom Lane  wrote:

> Zhihong Yu  writes:
> > On Thu, Mar 3, 2022 at 7:44 AM Tom Lane  wrote:
> >> Zhihong Yu  writes:
> >>> In test output, I saw:
> >>> src/backend/utils/adt/tid.c:112:16: runtime error: left shift of 65535
> by
> >>> 16 places cannot be represented in type 'int'
>
> > Jenkins build is alma8-clang12-asan
>
> Oh, I misread this as a compile-time warning, but it must be from ASAN.
> Was the test case one of your own, or just our normal regression tests?
>
> (I think the code is indeed incorrect, but I'm wondering why this hasn't
> been reported before.  It's been like that for a long time.)
>
> regards, tom lane
>
Hi,
The Jenkins test is ported from contrib/postgres_fdw/sql/postgres_fdw.sql -
so theoretically PG would see the same error for clang12 on Alma.

Here were a few lines prior to the sanitizer complaint:

ts1|pid123867|:30045 2022-03-02 01:47:57.098 UTC [124161] STATEMENT:
 CREATE TRIGGER trig_row_before
ts1|pid123867|:30045BEFORE INSERT OR UPDATE OR DELETE ON rem1
ts1|pid123867|:30045FOR EACH ROW EXECUTE PROCEDURE
trigger_data(23,'skidoo');
ts1|pid123867|:30045 2022-03-02 01:47:57.106 UTC [124161] ERROR:  function
trigger_data() does not exist
ts1|pid123867|:30045 2022-03-02 01:47:57.106 UTC [124161] STATEMENT:
 CREATE TRIGGER trig_row_after
ts1|pid123867|:30045AFTER INSERT OR UPDATE OR DELETE ON rem1
ts1|pid123867|:30045FOR EACH ROW EXECUTE PROCEDURE
trigger_data(23,'skidoo');

I think the ASAN build on Alma is able to detect errors such as this.

Cheers

Re: pg_walinspect - a new extension to get raw WAL data and WAL stats

[Robert Haas]

On Fri, Feb 25, 2022 at 6:03 AM Bharath Rupireddy
 wrote:
> Added a new function that returns the first and last valid WAL record
> LSN of a given WAL file.

Sounds like fuzzy thinking to me. WAL records can cross file
boundaries, and forgetting about that leads to all sorts of problems.
Just give people one function that decodes a range of LSNs and call it
good. Why do you need anything else? If people want to get the first
record that begins in a segment or the first record any portion of
which is in a particular segment or the last record that begins in a
segment or the last record that ends in a segment or any other such
thing, they can use a WHERE clause for that... and if you think they
can't, then that should be good cause to rethink the return value of
the one-and-only SRF that I think you need here.

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

Re: casting operand to proper type in BlockIdGetBlockNumber

[Tom Lane]

Zhihong Yu  writes:
> On Thu, Mar 3, 2022 at 8:24 AM Tom Lane  wrote:
>> Oh, I misread this as a compile-time warning, but it must be from ASAN.
>> Was the test case one of your own, or just our normal regression tests?

> The Jenkins test is ported from contrib/postgres_fdw/sql/postgres_fdw.sql -
> so theoretically PG would see the same error for clang12 on Alma.

Hmph.  I tried enabling -fsanitize=undefined here, and I get some
complaints about passing null pointers to memcmp and the like, but
nothing about this shift (tested with clang 12.0.1 on RHEL8 as well
as clang 13.0.0 on Fedora 35).  What compiler switches are being
used exactly?

regards, tom lane

Re: Add 64-bit XIDs into PostgreSQL 15

[Stephen Frost]

Greetings,

* Pavel Borisov (pashkin.e...@gmail.com) wrote:
> > > BTW messages with patches in this thread are always invoke manual spam
> > moderation and we need to wait for ~3 hours before the message with patch
> > becomes visible in the hackers thread. Now when I've already answered
> > Alexander's letter with v10 patch the very message (and a patch) I've
> > answered is still not visible in the thread and to CFbot.
> > >
> > > Can something be done in hackers' moderation engine to make new versions
> > patches become visible hassle-free?
> >
> > Is your email address subscribed to the pgsql-hackers mailing list?
> > AFAIK, moderation is only applied for non-subscribers.
> 
> Yes, it is in the list. The problem is that patch is over 1Mb. So it
> strictly goes through moderation. And this is unchanged for 2 months
> already.

Right, >1MB will be moderated, as will emails that are CC'd to multiple
lists, and somehow this email thread ended up with two different
addresses for -hackers, which isn't good.

> I was advised to use .gz, which I will do next time.

Better would be to break the patch down into reasonable and independent
pieces for review and commit on separate threads as suggested previously
and not to send huge patches to the list with the idea that someone is
going to actually fully review and commit them.  That's just not likely
to end up working well anyway.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Proposal: Support custom authentication methods using hooks,Re: Proposal: Support custom authentication methods using hooks

[Stephen Frost]

Greetings,

* Tatsuo Ishii (is...@sraoss.co.jp) wrote:
> > On 2/25/22 12:39 PM, Tom Lane wrote:
> >> Jeff Davis  writes:
> >>> On Thu, 2022-02-24 at 20:47 -0500, Tom Lane wrote:
>  ... and, since we can't readily enforce that the client only sends
>  those cleartext passwords over suitably-encrypted connections, this
>  could easily be a net negative for security.  Not sure that I think
>  it's a good idea.
> >> 
> >>> I don't understand your point. Can't you just use "hostssl" rather
> >>> than
> >>> "host"?
> >> My point is that sending cleartext passwords over the wire is an
> >> insecure-by-definition protocol that we shouldn't be encouraging
> >> more use of.
> > 
> > This is my general feeling as well. We just spent a bunch of effort
> > adding, refining, and making SCRAM the default method. I think doing
> > anything that would drive more use of sending plaintext passwords,
> > even over TLS, is counter to that.
> 
> There's at least one use case to use plaintext passwords. Pgpool-II
> accepts plaintext passwords from frontend (from frontend's point of
> view, it looks as if the frontend speaks with PostgreSQL server which
> requests the plaintext password authentication), then negotiates with
> backends regarding authentication method they demand. Suppose we have
> 2 PostgreSQL clusters and they require md5 auth. They send different
> password encryption salt and Pgpool-II deal with each server using the
> salt and password. So Pgpool-II needs plaintext password.  Same thing
> can be said to SCRAM-SHA-256 authentication because it's kind of
> challenge/response based authentication.

Passing around plaintext passwords isn't good, regardless of what's
doing it.  That some things do that today is a problem, not something
that should be held up as an example use-case that we should be
retaining support for.

> Actually it is possible for Pgpool-II to not use plaintext passwords
> reading from frontend. In this case passwords are stored in a file and
> Pgpool-II reads passwords from the file. But this is annoying for
> users because they have to sync the passwords stored in PostgreSQL
> with the passwords stored in the file.

Users authenticating to an application and then independently having
applications authenticate to the database server is actually rather
common.  I agree that proxying credentials is generally better and I'm
hoping to commit support for exactly that during this CF for GSSAPI (aka
Kerberos), where it's cleanly supported.  Proxying using plaintext
passwords isn't good though.

> So, dropping plaintext password authentication support from libpq will
> make it impossible for users to use the former method.

Yes, just like dropping support for md5 would make it impossible for
users to have their passwords be hashed with md5, which is an altogether
good thing considering how easy it is to brute-force md5 these days.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Proposal: Support custom authentication methods using hooks

[Bruce Momjian]

On Thu, Mar  3, 2022 at 10:45:42AM +0100, Peter Eisentraut wrote:
> On 02.03.22 16:45, Jonathan S. Katz wrote:
> > By that argument, we should have kept "password" (plain) as an
> > authentication method.
> 
> For comparison, the time between adding md5 and removing password was 16
> years.  It has been 5 years since scram was added.

Uh, when did we remove "password".  I still see it mentioned in
pg_hba.conf.  Am I missing something?

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Re: casting operand to proper type in BlockIdGetBlockNumber

[Zhihong Yu]

On Thu, Mar 3, 2022 at 9:13 AM Tom Lane  wrote:

> Zhihong Yu  writes:
> > On Thu, Mar 3, 2022 at 8:24 AM Tom Lane  wrote:
> >> Oh, I misread this as a compile-time warning, but it must be from ASAN.
> >> Was the test case one of your own, or just our normal regression tests?
>
> > The Jenkins test is ported from
> contrib/postgres_fdw/sql/postgres_fdw.sql -
> > so theoretically PG would see the same error for clang12 on Alma.
>
> Hmph.  I tried enabling -fsanitize=undefined here, and I get some
> complaints about passing null pointers to memcmp and the like, but
> nothing about this shift (tested with clang 12.0.1 on RHEL8 as well
> as clang 13.0.0 on Fedora 35).  What compiler switches are being
> used exactly?
>
> regards, tom lane
>
Hi,
This is from (internal Jenkins) build log:

CMAKE_C_FLAGS  -Werror -fno-strict-aliasing -Wall -msse4.2 -Winvalid-pch
-pthread -DBOOST_BIND_NO_PLACEHOLDERS
-DBOOST_UUID_RANDOM_PROVIDER_FORCE_POSIX -DROCKSDB_PLATFORM_POSIX
-DBOOST_ERROR_CODE_HEADER_ONLY -march=ivybridge -mcx16
-DYB_COMPILER_TYPE=clang12 -DYB_COMPILER_VERSION=12.0.1
-DROCKSDB_LIB_IO_POSIX -DSNAPPY -DLZ4 -DZLIB -mno-avx -mno-bmi -mno-bmi2
-mno-fma -D__STDC_FORMAT_MACROS -Wno-deprecated-declarations
-DGFLAGS=gflags  -Werror=enum-compare  -Werror=switch -Werror=return-type
 -Werror=string-plus-int -Werror=return-stack-address
-Werror=implicit-fallthrough -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS
-Wthread-safety-analysis -Wshorten-64-to-32 -ggdb -O1
-fno-omit-frame-pointer -DFASTDEBUG -Wno-ambiguous-member-template
-Wimplicit-fallthrough -Qunused-arguments -stdlib=libc++
-D_GLIBCXX_EXTERN_TEMPLATE=0 -nostdinc++ -stdlib=libc++
-D_GLIBCXX_EXTERN_TEMPLATE=0 -nostdinc++ -shared-libasan -fsanitize=address
-DADDRESS_SANITIZER -fsanitize=undefined -fno-sanitize-recover=all
-fno-sanitize=alignment,vptr -fsanitize-recover=float-cast-overflow
-fsanitize-blacklist=... -fPIC

I would suggest trying out the build on Alma Linux.

FYI

Re: casting operand to proper type in BlockIdGetBlockNumber

[Andres Freund]

Hi,

On 2022-03-03 12:13:40 -0500, Tom Lane wrote:
> Zhihong Yu  writes:
> > On Thu, Mar 3, 2022 at 8:24 AM Tom Lane  wrote:
> >> Oh, I misread this as a compile-time warning, but it must be from ASAN.
> >> Was the test case one of your own, or just our normal regression tests?
> 
> > The Jenkins test is ported from contrib/postgres_fdw/sql/postgres_fdw.sql -
> > so theoretically PG would see the same error for clang12 on Alma.
> 
> Hmph.  I tried enabling -fsanitize=undefined here, and I get some
> complaints about passing null pointers to memcmp and the like, but
> nothing about this shift (tested with clang 12.0.1 on RHEL8 as well
> as clang 13.0.0 on Fedora 35).

We should fix these passing-null-pointer cases...


> What compiler switches are being used exactly?

FWIW, I've successfully used:
-Og -fsanitize=alignment,undefined -fno-sanitize=nonnull-attribute 
-fno-sanitize=float-cast-overflow -fno-sanitize-recover=all

Need to manually add -ldl, because -fsanitize breaks our dl test (it uses
dlopen, but not dlsym). Was planning to submit a fix for that...

Greetings,

Andres Freund

Re: Proposal: Support custom authentication methods using hooks

[Stephen Frost]

Greetings,

* Tatsuo Ishii (is...@sraoss.co.jp) wrote:
> > Yes, really, it's a known-broken system which suffers from such an old
> > and well known attack that it's been given a name: pass-the-hash.  As
> > was discussed on this thread even, just the fact that it's not trivial
> > to break on the wire doesn't make it not-broken, particularly when we
> > use the username (which is rather commonly the same one used across
> > multiple systems..) as the salt.  Worse, md5 isn't exactly the pinnacle
> 
> I am not a big fan of md5 auth but saying that md5 auth uses username
> as the salt is oversimplified. The md5 hashed password shored in
> pg_shadow is created as md5(password + username).  But the md5 hashed
> password flying over wire is using a random salt like md5(md5(password
> + username) + random_salt).

Err, no, it's not oversimplified at all- we do, in fact, as you say
above, use the username as the salt for what gets stored in pg_authid
(pg_shadow is just a view).  That's absolutely a problem because servers
can be compromised, backups can be compromised, and when it comes to PG
servers you don't even need to actually bother cracking the password
once you've gained access to an md5 value in pg_authid anyway.

Yes, we do use a challenge/response over the wire but that doesn't
absolve us of the fact that the hashes we store in pg_authid with the
md5 method is subject to pass-the-hash and brute-force attacks against
it.  If anything, the challenge/response over the wire is less useful
considering the common usage of TLS these days.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Proposal: Support custom authentication methods using hooks

[Jonathan S. Katz]

On 3/3/22 12:23 PM, Bruce Momjian wrote:

On Thu, Mar  3, 2022 at 10:45:42AM +0100, Peter Eisentraut wrote:

On 02.03.22 16:45, Jonathan S. Katz wrote:

By that argument, we should have kept "password" (plain) as an
authentication method.


For comparison, the time between adding md5 and removing password was 16
years.  It has been 5 years since scram was added.


Uh, when did we remove "password".  I still see it mentioned in
pg_hba.conf.  Am I missing something?


I may have explained this wrong. The protocol still supports "plain" but 
we removed the ability to store passwords in plaintext:


"Remove the ability to store unencrypted passwords on the server

"The password_encryption server parameter no longer supports off or 
plain. The UNENCRYPTED option is no longer supported in CREATE/ALTER 
USER ... PASSWORD. Similarly, the --unencrypted option has been removed 
from createuser. Unencrypted passwords migrated from older versions will 
be stored encrypted in this release. The default setting for 
password_encryption is still md5."


Jonathan

[1] https://www.postgresql.org/docs/release/10.0/



OpenPGP_signature
Description: OpenPGP digital signature

Re: Proposal: Support custom authentication methods using hooks

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@enterprisedb.com) wrote:
> On 02.03.22 21:49, samay sharma wrote:
> >I think we are discussing two topics in this thread which in my opinion
> >are orthogonal.
> >
> >(a) Should we make authentication methods pluggable by exposing these
> >hooks? - These will allow users to add plugins of their own to support
> >whatever auth method they like. One immediate use case (and what prompted
> >me to start looking at this) is Azure Active Directory integration which
> >is a common request from Azure customers. We could also, over time, move
> >some of our existing auth methods into extensions if we don’t want to
> >maintain them in core.
> 
> I don't think people are necessarily opposed to that.

I'm not thrilled with it, at least.  It's not clear that just backend
hooks would be enough either- certainly a number of our existing
mechanisms require support in libpq and those are generally the ones
that are more secure too (GSSAPI, Certs), so how would that work with
something that's 'pluggable'?  Are we going to have libpq loading in
libraries too?

> At the moment, it is not possible to judge whether the hook interface you
> have chosen is appropriate.

Agreed.

> I suggest you actually implement the Azure provider, then make the hook
> interface, and then show us both and we can see what to do with it.

Better- implement a standard that's also supported by Azure and not
something proprietary that can't be evaluated or which hasn't been
reviewed by security experts.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Problem with moderation of messages with patched attached.

[Stephen Frost]

Greetings,

* Aleksander Alekseev (aleksan...@timescale.com) wrote:
> My last email to pgsql-jobs@ was moderated in a similar fashion. To my
> knowledge that mailing list is not pre-moderated. So it may have the same
> problem, and not only with patches. (We use regular Google Workspace.)

-jobs is moderated.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Problem with moderation of messages with patched attached.

[Stephen Frost]

Greetings,

* Pavel Borisov (pashkin.e...@gmail.com) wrote:
> > On Thu, 3 Mar 2022 at 13:28, Pavel Borisov  wrote:
> >> The mail system doesn't have the capability to apply different moderation
> >>> rules for people in that way I'm afraid.
> >>>
> >> Maybe then 2MB for everyone? Otherwise it's not so convenient. Lead to
> >> answers before the questions in the thread [1], seems weird.
> >
> > Then someone will complain if their patch is 2.1MB! How often are messages
> > legitimately over 1MB anyway, even with a patch? I don't usually moderate
> > -hackers, so I don't know if this is a common thing or not.

I do pay attention to -hackers and no, it doesn't come up very often.

> Authors in the mentioned thread [1] bump into this issue while posting all
> 11 versions of a patchset. It is little bit more than 1MB. We can try to
> use .gz and if this doesn't work we report it again.

This patch set really shoudl be broken down into smaller independent
pieces that attack different parts and not be all one big series of
patches.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: casting operand to proper type in BlockIdGetBlockNumber

[Tom Lane]

Andres Freund  writes:
> On 2022-03-03 12:13:40 -0500, Tom Lane wrote:
>> Hmph.  I tried enabling -fsanitize=undefined here, and I get some
>> complaints about passing null pointers to memcmp and the like, but
>> nothing about this shift (tested with clang 12.0.1 on RHEL8 as well
>> as clang 13.0.0 on Fedora 35).

> We should fix these passing-null-pointer cases...

Yeah, working on that now.  But I'm pretty confused about why I can't
duplicate this shift complaint.  Alma is a Red Hat clone no?  Why
doesn't its compiler act the same as RHEL8's?

> Need to manually add -ldl, because -fsanitize breaks our dl test (it uses
> dlopen, but not dlsym). Was planning to submit a fix for that...

Hmm ... didn't get through check-world yet, but I don't see that
so far.

regards, tom lane

Re: casting operand to proper type in BlockIdGetBlockNumber

[Andres Freund]

Hi,

On 2022-03-03 12:45:22 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2022-03-03 12:13:40 -0500, Tom Lane wrote:
> >> Hmph.  I tried enabling -fsanitize=undefined here, and I get some
> >> complaints about passing null pointers to memcmp and the like, but
> >> nothing about this shift (tested with clang 12.0.1 on RHEL8 as well
> >> as clang 13.0.0 on Fedora 35).
> 
> > We should fix these passing-null-pointer cases...
> 
> Yeah, working on that now.  But I'm pretty confused about why I can't
> duplicate this shift complaint.  Alma is a Red Hat clone no?  Why
> doesn't its compiler act the same as RHEL8's?

I didn't see that either. It could be a question of building with full
optimizations / asserts vs without?


> > Need to manually add -ldl, because -fsanitize breaks our dl test (it uses
> > dlopen, but not dlsym). Was planning to submit a fix for that...
> 
> Hmm ... didn't get through check-world yet, but I don't see that
> so far.

Oh, for me it doesn't even build. Perhaps one of the dependencies injects it
as well?

Greetings,

Andres Freund

Re: Proposal: Support custom authentication methods using hooks

[Bruce Momjian]

On Thu, Mar  3, 2022 at 12:38:32PM -0500, Jonathan Katz wrote:
> On 3/3/22 12:23 PM, Bruce Momjian wrote:
> > On Thu, Mar  3, 2022 at 10:45:42AM +0100, Peter Eisentraut wrote:
> > > On 02.03.22 16:45, Jonathan S. Katz wrote:
> > > > By that argument, we should have kept "password" (plain) as an
> > > > authentication method.
> > > 
> > > For comparison, the time between adding md5 and removing password was 16
> > > years.  It has been 5 years since scram was added.
> > 
> > Uh, when did we remove "password".  I still see it mentioned in
> > pg_hba.conf.  Am I missing something?
> 
> I may have explained this wrong. The protocol still supports "plain" but we
> removed the ability to store passwords in plaintext:
> 
> "Remove the ability to store unencrypted passwords on the server
> 
> "The password_encryption server parameter no longer supports off or plain.
> The UNENCRYPTED option is no longer supported in CREATE/ALTER USER ...
> PASSWORD. Similarly, the --unencrypted option has been removed from
> createuser. Unencrypted passwords migrated from older versions will be
> stored encrypted in this release. The default setting for
> password_encryption is still md5."

OK, that does make sense.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Re: wrong fds used for refilenodes after pg_upgrade relfilenode changes Reply-To:

[Robert Haas]

On Wed, Mar 2, 2022 at 3:00 PM Andres Freund  wrote:
> On 2022-03-02 14:52:01 -0500, Robert Haas wrote:
> > - I am having some trouble understanding clearly what 0001 is doing.
> > I'll try to study it further.
>
> It tests for the various scenarios I could think of that could lead to FD
> reuse, to state the obvious ;). Anything particularly unclear.

As I understand it, the basic idea of this test is:

1. create a database called 'conflict_db' containing a table called 'large'
2. also create a table called 'replace_sb' in the postgres db.
3. have a long running transaction open in 'postgres' database that
repeatedly accesses the 'replace_sb' table to evict the 'conflict_db'
table, sometimes causing write-outs
4. try to get it to write out the data to a stale file descriptor by
fiddling with various things, and then detect the corruption that
results
5. use both a primary and a standby not because they are intrinsically
necessary but because you want to test that both cases work

As to (1) and (2), I note that 'large' is not a hugely informative
table name, especially when it's the smaller of the two test tables.
And also, the names are all over the place. The table names don't have
much to do with each other (large, replace_sb) and they're completely
unrelated to the database names (postgres, conflict_db). And then you
have Perl functions whose names also don't make it obvious what we're
talking about. I can't remember that verify() is the one that accesses
conflict.db large while cause_eviction() is the one that accesses
postgres.replace_sb for more than like 15 seconds. It'd make it a lot
easier if the table names, database names, and Perl function names had
some common elements.

As to (5), the identifiers are just primary and standby, without
mentioning the database name, which adds to the confusion, and there
are no comments explaining why we have two nodes.

Also, some of the comments seem like they might be in the wrong place:

+# Create template database with a table that we'll update, to trigger dirty
+# rows. Using a template database + preexisting rows makes it a bit easier to
+# reproduce, because there's no cache invalidations generated.

Right after this comment, we create a table and then a template
database just after, so I think we're not avoiding any invalidations
at this point. We're avoiding them at later points where the comments
aren't talking about this.

I think it would be helpful to find a way to divide the test case up
into sections that are separated from each other visually, and explain
the purpose of each test a little more in a comment. For example I'm
struggling to understand the exact purpose of this bit:

+$node_primary->safe_psql('conflict_db', "VACUUM FULL large;");
+$node_primary->safe_psql('conflict_db', "UPDATE large SET datab = 3;");
+
+verify($node_primary, $node_standby, 3,
+  "restored contents as expected");

I'm all for having test coverage of VACUUM FULL, but it isn't clear to
me what this does that is related to FD reuse.

Similarly for the later ones. I generally grasp that you are trying to
make sure that things are OK with respect to FD reuse in various
scenarios, but I couldn't explain to you what we'd be losing in terms
of test coverage if we removed this line:

+$node_primary->safe_psql('conflict_db', "UPDATE large SET datab = 7;");

I am not sure how much all of this potential cleanup matters because
I'm pretty skeptical that this would be stable in the buildfarm. It
relies on a lot of assumptions about timing and row sizes and details
of when invalidations are sent that feel like they might not be
entirely predictable at the level you would need to have this
consistently pass everywhere. Perhaps I am too pessimistic?

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

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

[Julien Rouhaud]

On Wed, Mar 2, 2022 at 7:15 PM Nitin Jadhav
 wrote:
>
> > > > As mentioned upthread, there can be multiple backends that request a
> > > > checkpoint, so unless we want to store an array of pid we should store 
> > > > a number
> > > > of backend that are waiting for a new checkpoint.
>
> It's a good metric to show in the view but the information is not
> readily available. Additional code is required to calculate the number
> of requests. Is it worth doing that? I feel this can be added later if
> required.

Is it that hard or costly to do?  Just sending a message to increment
the stat counter in RequestCheckpoint() would be enough.

Also, unless I'm missing something it's still only showing the initial
checkpoint flags, so it's *not* showing what the checkpoint is really
doing, only what the checkpoint may be doing if nothing else happens.
It just feels wrong.  You could even use that ckpt_flags info to know
that at least one backend has requested a new checkpoint, if you don't
want to have a number of backends.

Re: wrong fds used for refilenodes after pg_upgrade relfilenode changes Reply-To:

[Andres Freund]

Hi,

On 2022-03-03 13:11:17 -0500, Robert Haas wrote:
> On Wed, Mar 2, 2022 at 3:00 PM Andres Freund  wrote:
> > On 2022-03-02 14:52:01 -0500, Robert Haas wrote:
> > > - I am having some trouble understanding clearly what 0001 is doing.
> > > I'll try to study it further.
> >
> > It tests for the various scenarios I could think of that could lead to FD
> > reuse, to state the obvious ;). Anything particularly unclear.
> 
> As I understand it, the basic idea of this test is:
> 
> 1. create a database called 'conflict_db' containing a table called 'large'
> 2. also create a table called 'replace_sb' in the postgres db.
> 3. have a long running transaction open in 'postgres' database that
> repeatedly accesses the 'replace_sb' table to evict the 'conflict_db'
> table, sometimes causing write-outs
> 4. try to get it to write out the data to a stale file descriptor by
> fiddling with various things, and then detect the corruption that
> results
> 5. use both a primary and a standby not because they are intrinsically
> necessary but because you want to test that both cases work

Right.


I wasn't proposing to commit the test as-is, to be clear. It was meant as a
demonstration of the types of problems I can see, and what's needed to fix
them...


> I can't remember that verify() is the one that accesses conflict.db large
> while cause_eviction() is the one that accesses postgres.replace_sb for more
> than like 15 seconds.

For more than 15seconds? The whole test runs in a few seconds for me...


> As to (5), the identifiers are just primary and standby, without
> mentioning the database name, which adds to the confusion, and there
> are no comments explaining why we have two nodes.

I don't follow this one - physical rep doesn't do anything below the database
level?



> I think it would be helpful to find a way to divide the test case up
> into sections that are separated from each other visually, and explain
> the purpose of each test a little more in a comment. For example I'm
> struggling to understand the exact purpose of this bit:
> 
> +$node_primary->safe_psql('conflict_db', "VACUUM FULL large;");
> +$node_primary->safe_psql('conflict_db', "UPDATE large SET datab = 3;");
> +
> +verify($node_primary, $node_standby, 3,
> +  "restored contents as expected");
> 
> I'm all for having test coverage of VACUUM FULL, but it isn't clear to
> me what this does that is related to FD reuse.

It's just trying to clean up prior damage, so the test continues to later
ones. Definitely not needed once there's a fix. But it's useful while trying
to make the test actually detect various corruptions.


> Similarly for the later ones. I generally grasp that you are trying to
> make sure that things are OK with respect to FD reuse in various
> scenarios, but I couldn't explain to you what we'd be losing in terms
> of test coverage if we removed this line:
> 
> +$node_primary->safe_psql('conflict_db', "UPDATE large SET datab = 7;");
> 
> I am not sure how much all of this potential cleanup matters because
> I'm pretty skeptical that this would be stable in the buildfarm.

Which "potential cleanup" are you referring to?


> It relies on a lot of assumptions about timing and row sizes and details of
> when invalidations are sent that feel like they might not be entirely
> predictable at the level you would need to have this consistently pass
> everywhere. Perhaps I am too pessimistic?

I don't think the test passing should be dependent on row size, invalidations
etc to a significant degree (unless the tables are too small to reach s_b
etc)? The exact symptoms of failures are highly unstable, but obviously we'd
fix them in-tree before committing a test. But maybe I'm missing a dependency?

FWIW, the test did pass on freebsd, linux, macos and windows with the
fix. After a few iterations of improving the fix ;)

Greetings,

Andres Freund

Re: wrong fds used for refilenodes after pg_upgrade relfilenode changes Reply-To:

[Robert Haas]

On Thu, Mar 3, 2022 at 1:28 PM Andres Freund  wrote:
> > I can't remember that verify() is the one that accesses conflict.db large
> > while cause_eviction() is the one that accesses postgres.replace_sb for more
> > than like 15 seconds.
>
> For more than 15seconds? The whole test runs in a few seconds for me...

I'm not talking about running the test. I'm talking about reading it
and trying to keep straight what is happening. The details of which
function is accessing which database keep going out of my head.
Quickly. Maybe because I'm dumb, but I think some better naming could
help, just in case other people are dumb, too.

> > As to (5), the identifiers are just primary and standby, without
> > mentioning the database name, which adds to the confusion, and there
> > are no comments explaining why we have two nodes.
>
> I don't follow this one - physical rep doesn't do anything below the database
> level?

Right but ... those handles are connected to a particular DB on that
node, not just the node in general.

> > I think it would be helpful to find a way to divide the test case up
> > into sections that are separated from each other visually, and explain
> > the purpose of each test a little more in a comment. For example I'm
> > struggling to understand the exact purpose of this bit:
> >
> > +$node_primary->safe_psql('conflict_db', "VACUUM FULL large;");
> > +$node_primary->safe_psql('conflict_db', "UPDATE large SET datab = 3;");
> > +
> > +verify($node_primary, $node_standby, 3,
> > +  "restored contents as expected");
> >
> > I'm all for having test coverage of VACUUM FULL, but it isn't clear to
> > me what this does that is related to FD reuse.
>
> It's just trying to clean up prior damage, so the test continues to later
> ones. Definitely not needed once there's a fix. But it's useful while trying
> to make the test actually detect various corruptions.

Ah, OK, I definitely did not understand that before.

> > Similarly for the later ones. I generally grasp that you are trying to
> > make sure that things are OK with respect to FD reuse in various
> > scenarios, but I couldn't explain to you what we'd be losing in terms
> > of test coverage if we removed this line:
> >
> > +$node_primary->safe_psql('conflict_db', "UPDATE large SET datab = 7;");
> >
> > I am not sure how much all of this potential cleanup matters because
> > I'm pretty skeptical that this would be stable in the buildfarm.
>
> Which "potential cleanup" are you referring to?

I mean, whatever improvements you might consider making based on my comments.

> > It relies on a lot of assumptions about timing and row sizes and details of
> > when invalidations are sent that feel like they might not be entirely
> > predictable at the level you would need to have this consistently pass
> > everywhere. Perhaps I am too pessimistic?
>
> I don't think the test passing should be dependent on row size, invalidations
> etc to a significant degree (unless the tables are too small to reach s_b
> etc)? The exact symptoms of failures are highly unstable, but obviously we'd
> fix them in-tree before committing a test. But maybe I'm missing a dependency?
>
> FWIW, the test did pass on freebsd, linux, macos and windows with the
> fix. After a few iterations of improving the fix ;)

Hmm. Well, I admit that's already more than I would have expected

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

Re: casting operand to proper type in BlockIdGetBlockNumber

[Tom Lane]

I wrote:
> Andres Freund  writes:
>> We should fix these passing-null-pointer cases...

> Yeah, working on that now.

The attached is enough to get through check-world with
"-fsanitize=undefined" using RHEL8's clang 12.0.1.
Most of it is the same old null-pointer-with-zero-count
business, but the change in numeric.c is a different
issue: "ln(-1.0)" ends up computing log10(0), which
produces -Inf, and then tries to assign that to an integer.
We don't actually care about the garbage result in that case,
so it's only a sanitizer complaint not a live bug.

I'm not sure whether to back-patch --- looking through the
git logs, it seems we've back-patched some fixes like these
and not others.  Thoughts?

In any case, if we're going to take this seriously it seems
like we need a buildfarm machine or two testing this option.

regards, tom lane

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 59d43e2ba9..4e6aeba315 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -328,7 +328,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
 	/*
 	 * copy the scan key, if appropriate
 	 */
-	if (key != NULL)
+	if (key != NULL && scan->rs_base.rs_nkeys > 0)
 		memcpy(scan->rs_base.rs_key, key, scan->rs_base.rs_nkeys * sizeof(ScanKeyData));
 
 	/*
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index ceadac70d5..ff0b8a688d 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -1564,8 +1564,8 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
 static bool
 TransactionIdInArray(TransactionId xid, TransactionId *xip, Size num)
 {
-	return bsearch(&xid, xip, num,
-   sizeof(TransactionId), xidComparator) != NULL;
+	return num > 0 &&
+		bsearch(&xid, xip, num, sizeof(TransactionId), xidComparator) != NULL;
 }
 
 /*
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index de787c3d37..3d9088a704 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -297,8 +297,9 @@ TransactionIdSetPageStatus(TransactionId xid, int nsubxids,
 	if (all_xact_same_page && xid == MyProc->xid &&
 		nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT &&
 		nsubxids == MyProc->subxidStatus.count &&
-		memcmp(subxids, MyProc->subxids.xids,
-			   nsubxids * sizeof(TransactionId)) == 0)
+		(nsubxids == 0 ||
+		 memcmp(subxids, MyProc->subxids.xids,
+nsubxids * sizeof(TransactionId)) == 0))
 	{
 		/*
 		 * If we can immediately acquire XactSLRULock, we update the status of
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index adf763a8ea..8964ddf3eb 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5353,8 +5353,9 @@ SerializeTransactionState(Size maxsize, char *start_address)
 	{
 		if (FullTransactionIdIsValid(s->fullTransactionId))
 			workspace[i++] = XidFromFullTransactionId(s->fullTransactionId);
-		memcpy(&workspace[i], s->childXids,
-			   s->nChildXids * sizeof(TransactionId));
+		if (s->nChildXids > 0)
+			memcpy(&workspace[i], s->childXids,
+   s->nChildXids * sizeof(TransactionId));
 		i += s->nChildXids;
 	}
 	Assert(i == nxids);
diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index 45b0dfc062..603cf9b0fa 100644
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -773,8 +773,11 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)
 
 		/* Copy as much as we can. */
 		Assert(mqh->mqh_partial_bytes + rb <= nbytes);
-		memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb);
-		mqh->mqh_partial_bytes += rb;
+		if (rb > 0)
+		{
+			memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb);
+			mqh->mqh_partial_bytes += rb;
+		}
 
 		/*
 		 * Update count of bytes that can be consumed, accounting for
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 975d7dcf47..45547f6ae7 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -10048,12 +10048,20 @@ exp_var(const NumericVar *arg, NumericVar *result, int rscale)
  *
  * Essentially, we're approximating log10(abs(ln(var))).  This is used to
  * determine the appropriate rscale when computing natural logarithms.
+ *
+ * Note: many callers call this before range-checking the input.  Therefore,
+ * we must be robust against values that are invalid to apply ln() to.
+ * We don't wish to throw an error here, so just return zero in such cases.
  */
 static int
 estimate_ln_dweight(const NumericVar *var)
 {
 	int			ln_dweight;
 
+	/* Caller should fail on ln(negative), but for the moment return zero */
+	if (var->sign != NUMERIC_POS)
+		return 0;
+
 	if (cmp_var(var, &const_zero_point_nine) >= 0 &&
 		cmp_var(var, &const_one_point_one) <= 0)
 	{
diff --git a/src/backend/utils/time/snapmgr.c 

Re: Commitfest 2022-03 Patch Triage Part 1b

[Greg Stark]

Just FYI. Better to follow up to the thread for the patch that's
already in the CF. Otherwise your patch will missed by someone who
looks at the CF entry to see the latest patch.

Re: [PATCH] Expose port->authn_id to extensions and triggers

[Jacob Champion]

On Wed, 2022-03-02 at 09:18 +0100, Peter Eisentraut wrote:
> On 01.03.22 23:05, Jacob Champion wrote:
> > On Tue, 2022-03-01 at 19:56 +0100, Peter Eisentraut wrote:
> > > This patch contains no documentation.  I'm having a hard time
> > > understanding what the name "session_authn_id" is supposed to convey.
> > > The comment for the Port.authn_id field says this is the "system
> > > username", which sounds like a clearer terminology.
> > 
> > "System username" may help from an internal development perspective,
> > especially as it relates to pg_ident.conf, but I don't think that's
> > likely to be a useful descriptor to an end user. (I don't think of a
> > client certificate's Subject Distinguished Name as a "system
> > username".) Does my attempt in v5 help?
> 
> Yeah, maybe there are better names.  But I have no idea what the letter 
> combination "authn_id" is supposed to stand for.  Is it an 
> "authentication identifier"? What does it identify?

Authenticated identity, but yeah, that's the gist. ("AuthN" being a
standard-ish way to differentiate authentication from "AuthZ"
authorization.)

It's meant to uniquely identify the end user in the case of usermaps,
where multiple separate entities might log in using the same role. It
is distinct from the authorized role name, though they might be exactly
the same in many common setups. And it's not set at all if no
authentication was done.

> Maybe I'm missing something here, but I don't find it clear.

I just used the internal name, but if we want to make it more clear
then now would be a good time. Do you have any suggestions? Does
expanding the name (pg_session_authenticated_id, or even
pg_session_authenticated_identity) help?

--Jacob

Re: [PATCH] Expose port->authn_id to extensions and triggers

[Jacob Champion]

On Thu, 2022-03-03 at 16:45 +0900, Michael Paquier wrote:
> Anyway, FixedParallelState
> includes some authentication data passed down by the leader when
> spawning a worker.  So, if we were to pass down the authn, we are
> going to need a new PARALLEL_KEY_* to serialize and restore the data
> passed down via a DSM like any other states as per the business in
> parallel.c.  Jacob, what do you think?

I guess it depends on what we want MyProcPort to look like in a
parallel worker. Are we comfortable having most of it be blank/useless?
Does it need to be filled in?

--Jacob

Re: casting operand to proper type in BlockIdGetBlockNumber

[Andres Freund]

Hi,

On 2022-03-03 14:00:14 -0500, Tom Lane wrote:
> The attached is enough to get through check-world with
> "-fsanitize=undefined" using RHEL8's clang 12.0.1.

Cool.


> I'm not sure whether to back-patch --- looking through the
> git logs, it seems we've back-patched some fixes like these
> and not others.  Thoughts?

It'd be easier to run a BF animal if we fixed it everywhere.


> In any case, if we're going to take this seriously it seems like we need a
> buildfarm machine or two testing this option.

I was planning to add it to the CI runs, just didn't have energy to fix the
failures yet. But you just did (although I think there might be failure or two
more on new-ish debians).

For the buildfarm, I could enable it on flaviventris? That runs an
experimental gcc, without optimization (whereas serinus runs with
optimization). Which seems reasonable to combine with sanitizers?

For CI I compared the cost of the different sanitizers. It looks like
alignment sanitizer is almost free, undefined is pretty cheap, and address
sanitizer is pretty expensive (but still much cheaper than valgrind).

Greetings,

Andres Freund

PS: Hm, seems mylodon died a while ago... Need to check what's up with that.

Re: row filtering for logical replication

[Euler Taveira]

On Thu, Mar 3, 2022, at 7:47 AM, Amit Kapila wrote:
> LGTM. I'll push this tomorrow unless Tomas or Euler feels otherwise.
Sounds good to me.


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

Re: [Proposal] Global temporary tables

[Greg Stark]

It doesn't look like this is going to get committed this release
cycle. I understand more feedback could be valuable, especially on the
overall design, but as this is the last commitfest of the release we
should focus on other patches for now and spend that time in the next
release cycle.

I'm going to bump this one now as Waiting on Author for the design
documentation Robert asks for and probably a plan for how to separate
that design into multiple separable features as Andres suggested.

I'm still hopeful we get to advance this early in 16 because I think
everyone agrees the feature would be great.

Re: casting operand to proper type in BlockIdGetBlockNumber

[Tom Lane]

Andres Freund  writes:
> On 2022-03-03 14:00:14 -0500, Tom Lane wrote:
>> I'm not sure whether to back-patch --- looking through the
>> git logs, it seems we've back-patched some fixes like these
>> and not others.  Thoughts?

> It'd be easier to run a BF animal if we fixed it everywhere.

Fair enough, will BP.

>> In any case, if we're going to take this seriously it seems like we need a
>> buildfarm machine or two testing this option.

> For the buildfarm, I could enable it on flaviventris? That runs an
> experimental gcc, without optimization (whereas serinus runs with
> optimization). Which seems reasonable to combine with sanitizers?

Dunno.  I already found out that my Mac laptop (w/ clang 13) detects
the numeric.c problem but not any of the other ones.  The messages
on RHEL8 cite where the system headers declare memcmp and friends
with "attribute nonnull", so I'm betting that Apple's headers lack
that annotation.

I also tried adding the various -m switches shown in Zhihong's
CFLAGS setting, but that still didn't repro the Alma warning
for me.

So it definitely seems like it's *real* system dependent which of
these warnings you get :-(.

regards, tom lane

Re: [Proposal] Global temporary tables

[Robert Haas]

On Thu, Mar 3, 2022 at 3:29 PM Greg Stark  wrote:
> I'm still hopeful we get to advance this early in 16 because I think
> everyone agrees the feature would be great.

I'm not saying this patch can't make progress, but I think the chances
of this being ready to commit any time in the v16 release cycle, let
alone at the beginning, are low. This patch set has been around since
2019, and here Andres and I are saying it's not even really reviewable
in the shape that it's in. I have done some review of it previously,
BTW, but eventually I gave up because it just didn't seem like we were
making any progress. And then a long time after that people were still
finding many server crashes with relatively simple test cases.

I agree that the feature is desirable, but I think getting there is
going to require a huge amount of effort that may amount to a total
rewrite of the patch.

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

Re: pg_stop_backup() v2 incorrectly marked as proretset

[Robert Haas]

On Wed, Mar 2, 2022 at 9:35 AM Tom Lane  wrote:
> Yeah, there's plenty of precedent for that coding if you look around.
> I've not read the whole patch, but this snippet seems fine to me
> if there's also an #undef at the end of the function.

>From later emails, it sounds like that's not the common practice in
similar cases, and I don't personally see the point.

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

Re: casting operand to proper type in BlockIdGetBlockNumber

[Andres Freund]

Hi,

On 2022-03-03 15:31:51 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2022-03-03 14:00:14 -0500, Tom Lane wrote:
> > For the buildfarm, I could enable it on flaviventris? That runs an
> > experimental gcc, without optimization (whereas serinus runs with
> > optimization). Which seems reasonable to combine with sanitizers?
> 
> Dunno.  I already found out that my Mac laptop (w/ clang 13) detects
> the numeric.c problem but not any of the other ones.  The messages
> on RHEL8 cite where the system headers declare memcmp and friends
> with "attribute nonnull", so I'm betting that Apple's headers lack
> that annotation.

The sanitizers are documented to work best on linux... As flaviventris runs
linux, so I'm not sure what your concern is?

I think basically newer glibc versions have more annotations, so ubsan will
have more things to fail against. So it'd be good to have a fairly regularly
updated OS.


> I also tried adding the various -m switches shown in Zhihong's
> CFLAGS setting, but that still didn't repro the Alma warning
> for me.

The compilation flags make it look like it's from a run of yugabyte's fork,
rather than plain postgres.

The message says:
src/backend/utils/adt/tid.c:112:16: runtime error: left shift of 65535 by 16 
places cannot be represented in type 'int'

Afaics that means bi_hi is 65535. So either we're dealing with a very large
relation or BlockIdGetBlockNumber() is getting passed InvalidBlockNumber?

It might be enough to do something like
SELECT * FROM pg_class WHERE ctid = '(65535, 17)';
to trigger the problem?

Greetings,

Andres Freund

Re: casting operand to proper type in BlockIdGetBlockNumber

[Zhihong Yu]

On Thu, Mar 3, 2022 at 1:11 PM Andres Freund  wrote:

> Hi,
>
> On 2022-03-03 15:31:51 -0500, Tom Lane wrote:
> > Andres Freund  writes:
> > > On 2022-03-03 14:00:14 -0500, Tom Lane wrote:
> > > For the buildfarm, I could enable it on flaviventris? That runs an
> > > experimental gcc, without optimization (whereas serinus runs with
> > > optimization). Which seems reasonable to combine with sanitizers?
> >
> > Dunno.  I already found out that my Mac laptop (w/ clang 13) detects
> > the numeric.c problem but not any of the other ones.  The messages
> > on RHEL8 cite where the system headers declare memcmp and friends
> > with "attribute nonnull", so I'm betting that Apple's headers lack
> > that annotation.
>
> The sanitizers are documented to work best on linux... As flaviventris runs
> linux, so I'm not sure what your concern is?
>
> I think basically newer glibc versions have more annotations, so ubsan will
> have more things to fail against. So it'd be good to have a fairly
> regularly
> updated OS.
>
>
> > I also tried adding the various -m switches shown in Zhihong's
> > CFLAGS setting, but that still didn't repro the Alma warning
> > for me.
>
> The compilation flags make it look like it's from a run of yugabyte's fork,
> rather than plain postgres.
>
Hi,
I should mention that, the PG subtree in yugabyte is currently aligned with
PG 11.
There have been backports from PG 12, but code related to tid.c
and block.h, etc is the same with upstream PG.

The fdw tests are backported from PG as well.


>
> The message says:
> src/backend/utils/adt/tid.c:112:16: runtime error: left shift of 65535 by
> 16 places cannot be represented in type 'int'
>
> Afaics that means bi_hi is 65535. So either we're dealing with a very large
> relation or BlockIdGetBlockNumber() is getting passed InvalidBlockNumber?
>
> It might be enough to do something like
> SELECT * FROM pg_class WHERE ctid = '(65535, 17)';
> to trigger the problem?
>

The above syntax is not currently supported in yugabyte.

FYI

Re: [Proposal] Global temporary tables

[Andres Freund]

Hi,

On 2022-03-03 16:07:37 -0500, Robert Haas wrote:
> On Thu, Mar 3, 2022 at 3:29 PM Greg Stark  wrote:
> > I'm still hopeful we get to advance this early in 16 because I think
> > everyone agrees the feature would be great.
> 
> I'm not saying this patch can't make progress, but I think the chances
> of this being ready to commit any time in the v16 release cycle, let
> alone at the beginning, are low. This patch set has been around since
> 2019, and here Andres and I are saying it's not even really reviewable
> in the shape that it's in. I have done some review of it previously,
> BTW, but eventually I gave up because it just didn't seem like we were
> making any progress. And then a long time after that people were still
> finding many server crashes with relatively simple test cases.
> 
> I agree that the feature is desirable, but I think getting there is
> going to require a huge amount of effort that may amount to a total
> rewrite of the patch.

Agreed. I think this needs very fundamental design work, and the patch itself
isn't worth reviewing until that's tackled.

Greetings,

Andres Freund

Re: pg_stop_backup() v2 incorrectly marked as proretset

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 2, 2022 at 9:35 AM Tom Lane  wrote:
>> I've not read the whole patch, but this snippet seems fine to me
>> if there's also an #undef at the end of the function.

>> From later emails, it sounds like that's not the common practice in
> similar cases, and I don't personally see the point.

The point is to make it clear that the macro isn't intended to affect
code outside the function.  Since C lacks block-scoped macros,
there's no other way to do that.

I concede that a lot of our code is pretty sloppy about this, but
that doesn't make it a good practice.

regards, tom lane

Re: casting operand to proper type in BlockIdGetBlockNumber

[Tom Lane]

Andres Freund  writes:
> The message says:
> src/backend/utils/adt/tid.c:112:16: runtime error: left shift of 65535 by 16 
> places cannot be represented in type 'int'

> Afaics that means bi_hi is 65535. So either we're dealing with a very large
> relation or BlockIdGetBlockNumber() is getting passed InvalidBlockNumber?

Presumably the latter, since we surely aren't using any terabyte-size
relations in our tests.

> It might be enough to do something like
> SELECT * FROM pg_class WHERE ctid = '(65535, 17)';
> to trigger the problem?

I tried to provoke it with cases like

# select '(-1,0)'::tid;
  tid   

 (4294967295,0)
(1 row)

# select '(40,1)'::tid;
  tid   

 (40,1)
(1 row)

without success.

On a nearby topic, I see that tidin's overflow checks are somewhere
between sloppy and nonexistent:

# select '(400,1)'::tid;
  tid   

 (1345294336,1)
(1 row)

I think I'll fix that while I'm looking at it ... but it still
doesn't explain why no complaint in tidout.

regards, tom lane

Re: pg_stop_backup() v2 incorrectly marked as proretset

[Chapman Flack]

On 03/03/22 16:40, Tom Lane wrote:
> The point is to make it clear that the macro isn't intended to affect
> code outside the function.  Since C lacks block-scoped macros,
> there's no other way to do that.
> 
> I concede that a lot of our code is pretty sloppy about this, but
> that doesn't make it a good practice.

Would the

  Datum values[3];
  bool   nulls[ lengthof(values) ];

pattern be more notationally tidy? No macro to define or undefine,
we already define lengthof() in c.h, and it seems pretty much made
for the purpose, if the objective is to have just one 3 to change
if it someday becomes not-3.

Regards,
-Chap

Re: pg_stop_backup() v2 incorrectly marked as proretset

[Tom Lane]

Chapman Flack  writes:
> On 03/03/22 16:40, Tom Lane wrote:
>> The point is to make it clear that the macro isn't intended to affect
>> code outside the function.  Since C lacks block-scoped macros,
>> there's no other way to do that.

> Would the

>   Datum values[3];
>   bool   nulls[ lengthof(values) ];

> pattern be more notationally tidy?

Hm, I don't care for that particularly.

(1) It *looks* asymmetrical, even if it isn't.

(2) I think a lot of the benefit of the macro approach is to give a name
(and thereby some free documentation, assuming you take some care in
choosing the name) to what would otherwise be a very anonymous constant.

There's an actual practical problem with the anonymous-constant approach,
which is that if you have some other occurrence of "3" in the function,
it's very hard to tell if that's indeed an independent value or it's
something that should have been replaced by lengthof(values).
Now admittedly the same complaint can be made against the macro
approach, but at least there, you have some chance of the macro's name
providing enough docs to make it clear what the proper uses of it are.
(I'd suggest that that other "3" should also have been made a named
constant in many cases.)

regards, tom lane

Re: Reducing power consumption on idle servers

[Tom Lane]

Jim Nasby  writes:
> I'm wondering if it'd be worth linking autovac wakeup from a truly idle 
> state to the stats collector. If there's no stats messages coming in 
> clearly there's nothing new for autovac.

That seems pretty scary in the current system design, where the
stats collector is intentionally not 100% reliable (and sometimes,
less intentionally, it fails completely).  If we get to a place
where we're willing to bank on stats being collected 100% of the
time, it might make sense.

regards, tom lane

Re: standby recovery fails (tablespace related) (tentative patch and discussion)

[Kyotaro Horiguchi]

At Wed, 02 Mar 2022 19:31:24 +0900 (JST), Kyotaro Horiguchi 
 wrote in 
> A function added to Util.pm used perl2host, which has been removed
> recently.

And same function contained a maybe-should-have-been-removed line
which makes Windows build unhappy.

This should make all platforms in the CI happy.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From ee17f0f4400ce484cdba80c84744ae47d68c6fa4 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 11 Nov 2021 20:42:00 +0900
Subject: [PATCH v18 1/3] Add tablespace support to TAP framework

TAP framework doesn't support nodes that have tablespaces.  Especially
backup and initialization from backups failed if the source node has
tablespaces.  This commit provides simple way to create tablespace
directories and allows backup routines to handle tablespaces.
---
 src/test/perl/PostgreSQL/Test/Cluster.pm | 264 ++-
 src/test/perl/PostgreSQL/Test/Utils.pm   |  42 
 2 files changed, 304 insertions(+), 2 deletions(-)

diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index be05845248..15d57b9a71 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -298,6 +298,64 @@ sub archive_dir
 
 =pod
 
+=item $node->tablespace_storage([, nocreate])
+
+Diretory to store tablespace directories.
+If nocreate is true, returns undef if not yet created.
+
+=cut
+
+sub tablespace_storage
+{
+	my ($self, $nocreate) = @_;
+
+	if (!defined $self->{_tsproot})
+	{
+		# tablespace is not used, return undef if nocreate is specified.
+		return undef if ($nocreate);
+
+		# create and remember the tablespae root directotry.
+		$self->{_tsproot} = PostgreSQL::Test::Utils::tempdir_short();
+	}
+
+	return $self->{_tsproot};
+}
+
+=pod
+
+=item $node->tablespaces()
+
+Returns a hash from tablespace OID to tablespace directory name.  For
+example, an oid 16384 pointing to /tmp/jWAhkT_fs0/ts1 is stored as
+$hash{16384} = "ts1".
+
+=cut
+
+sub tablespaces
+{
+	my ($self) = @_;
+	my $pg_tblspc = $self->data_dir . '/pg_tblspc';
+	my %ret;
+
+	# return undef if no tablespace is used
+	return undef if (!defined $self->tablespace_storage(1));
+
+	# collect tablespace entries in pg_tblspc directory
+	opendir(my $dir, $pg_tblspc);
+	while (my $oid = readdir($dir))
+	{
+		next if ($oid !~ /^([0-9]+)$/);
+		my $linkpath = "$pg_tblspc/$oid";
+		my $tsppath = PostgreSQL::Test::Utils::dir_readlink($linkpath);
+		$ret{$oid} = File::Basename::basename($tsppath);
+	}
+	closedir($dir);
+
+	return %ret;
+}
+
+=pod
+
 =item $node->backup_dir()
 
 The output path for backups taken with $node->backup()
@@ -313,6 +371,77 @@ sub backup_dir
 
 =pod
 
+=item $node->backup_tablespace_storage_path(backup_name)
+
+Returns tablespace location path for backup_name.
+Retuns the parent directory if backup_name is not given.
+
+=cut
+
+sub backup_tablespace_storage_path
+{
+	my ($self, $backup_name) = @_;
+	my $dir = $self->backup_dir . '/__tsps';
+
+	$dir .= "/$backup_name" if (defined $backup_name);
+
+	return $dir;
+}
+
+=pod
+
+=item $node->backup_create_tablespace_storage(backup_name)
+
+Create tablespace location directory for backup_name if not yet.
+Create the parent tablespace storage that holds all location
+directories if backup_name is not supplied.
+
+=cut
+
+sub backup_create_tablespace_storage
+{
+	my ($self, $backup_name) = @_;
+	my $dir = $self->backup_tablespace_storage_path($backup_name);
+
+	File::Path::make_path $dir if (! -d $dir);
+}
+
+=pod
+
+=item $node->backup_tablespaces(backup_name)
+
+Returns a reference to hash from tablespace OID to tablespace
+directory name of tablespace directory that the specified backup has.
+For example, an oid 16384 pointing to ../tsps/backup1/ts1 is stored as
+$hash{16384} = "ts1".
+
+=cut
+
+sub backup_tablespaces
+{
+	my ($self, $backup_name) = @_;
+	my $pg_tblspc = $self->backup_dir . '/' . $backup_name . '/pg_tblspc';
+	my %ret;
+
+	#return undef if this backup holds no tablespaces
+	return undef if (! -d $self->backup_tablespace_storage_path($backup_name));
+
+	# scan pg_tblspc directory of the backup
+	opendir(my $dir, $pg_tblspc);
+	while (my $oid = readdir($dir))
+	{
+		next if ($oid !~ /^([0-9]+)$/);
+		my $linkpath = "$pg_tblspc/$oid";
+		my $tsppath = PostgreSQL::Test::Utils::dir_readlink($linkpath);
+		$ret{$oid} = File::Basename::basename($tsppath);
+	}
+	closedir($dir);
+
+	return \%ret;
+}
+
+=pod
+
 =item $node->install_path()
 
 The configured install path (if any) for the node.
@@ -370,6 +499,7 @@ sub info
 	print $fh "Data directory: " . $self->data_dir . "\n";
 	print $fh "Backup directory: " . $self->backup_dir . "\n";
 	print $fh "Archive directory: " . $self->archive_dir . "\n";
+	print $fh "Tablespace directory: " . $self->tablespace_storage . "\n";
 	print $fh "Connection string: " . $self->connstr . "\n";
 	print $fh "Log file: " . $self->logfile . "\n";
 	print $fh "Install Path: ", $self->{_install_path} .

Re: Make mesage at end-of-recovery less scary.

[Kyotaro Horiguchi]

At Thu, 3 Mar 2022 15:39:44 +0530, Ashutosh Sharma  
wrote in 
> The new changes made in the patch look good. Thanks to the recent
> changes to speed WAL insertion that have helped us catch this bug.

Thanks for the quick checking.

> One small comment:
> 
> record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ);
> -   total_len = record->xl_tot_len;
> 
> Do you think we need to change the position of the comments written
> for above code that says:

Yeah, I didn't do that since it is about header verification.  But as
you pointed, the result still doesn't look perfect.

On second thought the two seems repeating the same things.  Thus I
merged the two comments together.  In this verion 16 it looks like
this.

>   /*
>* Validate the record header.
>*
>* Even though we use an XLogRecord pointer here, the whole record 
> header
>* might not fit on this page.  If the whole record header is on this 
> page,
>* validate it immediately.  Even otherwise xl_tot_len must be on this 
> page
>* (it is the first field of MAXALIGNed records), but we still cannot
>* access any further fields until we've verified that we got the whole
>* header, so do just a basic sanity check on record length, and 
> validate
>* the rest of the header after reading it from the next page.  The 
> length
>* check is necessary here to ensure that we enter the "Need to 
> reassemble
>* record" code path below; otherwise we might fail to apply
>* ValidXLogRecordHeader at all.
>*/
>   record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ);
>
>   if (targetRecOff <= XLOG_BLCKSZ - SizeOfXLogRecord)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 00d848df6bb8b9966dfbd39c98a388fda42a3e3c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Fri, 28 Feb 2020 15:52:58 +0900
Subject: [PATCH v16] Make End-Of-Recovery error less scary

When recovery in any type ends, we see a bit scary error message like
"invalid record length" that suggests something serious is
happening. Actually if recovery meets a record with length = 0, that
usually means it finished applying all available WAL records.

Make this message less scary as "reached end of WAL". Instead, raise
the error level for other kind of WAL failure to WARNING.
---
 src/backend/access/transam/xlogreader.c   | 144 +-
 src/backend/access/transam/xlogrecovery.c |  92 ++
 src/backend/replication/walreceiver.c |   7 +-
 src/bin/pg_waldump/pg_waldump.c   |  13 +-
 src/include/access/xlogreader.h   |   1 +
 src/test/recovery/t/011_crash_recovery.pl | 106 
 6 files changed, 305 insertions(+), 58 deletions(-)

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 35029cf97d..bd0f211a23 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -42,6 +42,8 @@ static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
 static int	ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
 			 int reqLen);
 static void XLogReaderInvalReadState(XLogReaderState *state);
+static bool ValidXLogRecordLength(XLogReaderState *state, XLogRecPtr RecPtr,
+  XLogRecord *record);
 static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
   XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
 static bool ValidXLogRecord(XLogReaderState *state, XLogRecord *record,
@@ -121,6 +123,7 @@ XLogReaderAllocate(int wal_segment_size, const char *waldir,
 		pfree(state);
 		return NULL;
 	}
+	state->EndOfWAL = false;
 	state->errormsg_buf[0] = '\0';
 
 	/*
@@ -292,6 +295,7 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
 	/* reset error state */
 	*errormsg = NULL;
 	state->errormsg_buf[0] = '\0';
+	state->EndOfWAL = false;
 
 	ResetDecoder(state);
 	state->abortedRecPtr = InvalidXLogRecPtr;
@@ -371,25 +375,21 @@ restart:
 	Assert(pageHeaderSize <= readOff);
 
 	/*
-	 * Read the record length.
+	 * Validate the record header.
 	 *
-	 * NB: Even though we use an XLogRecord pointer here, the whole record
-	 * header might not fit on this page. xl_tot_len is the first field of the
-	 * struct, so it must be on this page (the records are MAXALIGNed), but we
-	 * cannot access any other fields until we've verified that we got the
-	 * whole header.
-	 */
-	record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ);
-	total_len = record->xl_tot_len;
-
-	/*
-	 * If the whole record header is on this page, validate it immediately.
-	 * Otherwise do just a basic sanity check on xl_tot_len, and validate the
-	 * rest of the header after reading it from the next page.  The xl_tot_len
+	 * Even though we use an XLogRecord pointer here, the whole record header
+	 * might not fit on this page.  If the whole record header is on this page,
+	 * v

Re: pg_stop_backup() v2 incorrectly marked as proretset

[Michael Paquier]

On Thu, Mar 03, 2022 at 04:40:42PM -0500, Tom Lane wrote:
> The point is to make it clear that the macro isn't intended to affect
> code outside the function.  Since C lacks block-scoped macros,
> there's no other way to do that.
> 
> I concede that a lot of our code is pretty sloppy about this, but
> that doesn't make it a good practice.

Well, if we change that, better to do that in all the places where
this would be affected, but I am not sure to see a style appealing
enough on this thread.

From what I can see, history shows that the style of using a #define
for the number of columns originates from da2c1b8, aka 9.0.  Its use
inside a function originates from a755ea3 as of 9.1 and then it has
just spread around without any undefs, so it looks like people like
that way of doing things.
--
Michael


signature.asc
Description: PGP signature

Re: Add the replication origin name and commit-LSN to logical replication worker errcontext

[Masahiko Sawada]

On Thu, Mar 3, 2022 at 10:02 PM Masahiko Sawada  wrote:
>
>
> I'm updating the patches and will submit them.

Attached updated version patches.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/


v3-0002-Add-the-origin-name-and-remote-commit-LSN-to-logi.patch
Description: Binary data


v3-0001-Use-complete-sentences-in-logical-replication-wor.patch
Description: Binary data

Re: pg_stop_backup() v2 incorrectly marked as proretset

[Kyotaro Horiguchi]

At Fri, 4 Mar 2022 10:09:19 +0900, Michael Paquier  wrote 
in 
> On Thu, Mar 03, 2022 at 04:40:42PM -0500, Tom Lane wrote:
> > The point is to make it clear that the macro isn't intended to affect
> > code outside the function.  Since C lacks block-scoped macros,
> > there's no other way to do that.
> > 
> > I concede that a lot of our code is pretty sloppy about this, but
> > that doesn't make it a good practice.
> 
> Well, if we change that, better to do that in all the places where
> this would be affected, but I am not sure to see a style appealing
> enough on this thread.
> 
> From what I can see, history shows that the style of using a #define
> for the number of columns originates from da2c1b8, aka 9.0.  Its use
> inside a function originates from a755ea3 as of 9.1 and then it has
> just spread around without any undefs, so it looks like people like
> that way of doing things.

I'm one of them.  Not unliking #undef, though.

It seems to me the name "PG_STOP_BACKUP_V2_COLS" alone is specific
enough for the purpose of avoiding misuse.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: logical replication empty transactions

[Ajin Cherian]

I have split the patch into two. I have kept the logic of skipping
streaming changes in the second patch.
I will work on the second patch once we can figure out a solution for
the COMMIT PREPARED after restart problem.

regards,
Ajin Cherian


v23-0001-Skip-empty-transactions-for-logical-replication.patch
Description: Binary data


v23-0002-Skip-empty-streamed-transactions-for-logical-rep.patch
Description: Binary data

Re: XLogReadRecord() error in XlogReadTwoPhaseData()

[Noah Misch]

On Sun, Jan 16, 2022 at 01:02:41PM -0800, Noah Misch wrote:
> My next steps:
> 
> - Report a Debian bug for the sparc64+ext4 zeros problem.

Reported to Debian, then upstream:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1006157
https://marc.info/?t=16453926991

Last week, someone confirmed the behavior on kernel 5.17.0-rc5.  No other news.

Re: [PATCH] Expose port->authn_id to extensions and triggers

[Michael Paquier]

On Thu, Mar 03, 2022 at 07:16:17PM +, Jacob Champion wrote:
> I guess it depends on what we want MyProcPort to look like in a
> parallel worker. Are we comfortable having most of it be blank/useless?
> Does it need to be filled in?

Good question.  It depends on the definition of how much
authentication information makes sense for the parallel workers to
inherit from the parent.  And as I mentioned upthread, this definition
is not completely clear to me because the parallel workers do *not* go
through the authentication paths of the parent, they are just spawned
in their own dedicated paths that the leader invokes.  Inheriting all
this information from the leader has also an impact on the
PgBackendStatus entries of the workers as these are reported in
pg_stat_activity as far as I recall, and it could be confusing to see,
for example, some SSL or some GSS information for automatically
spawned processes because these don't use SSL or GSS when they pass
back data to the leader.

I have been looking at the commit history, and found about 6b7d11f
that switched all the functions of sslinfo to be parallel-restricted
especially because of this.  So if we inherit all this information the
restriction on the sslinfo functions could be lifted, though the
interest is honestly limited in this case.

postgres_fdw has introduced recently the concept of cached
connections, as of v14 with 411ae64 and 708d165, with a set of
parallel-restricted functions.  Some of the code paths related to
appname look at MyProcPort, so there could be a risk of having some
inconsistent information if this is accessed in a parallel worker.
Looking at the code, I don't think that it would happen now but
copying some of the data of MyProcPort to the worker could avoid any
future issues if this code gets extended.

At the end of the day, Port is an interface used for the communication
between the postmaster with the frontends, so I'd like to say that it
is correct to not apply this concept to parallel workers because they
are not designed to contact any frontend-side things.
--
Michael


signature.asc
Description: PGP signature

RE: Add the replication origin name and commit-LSN to logical replication worker errcontext

[osumi.takami...@fujitsu.com]

On Friday, March 4, 2022 10:09 AM Masahiko Sawada  wrote:
> On Thu, Mar 3, 2022 at 10:02 PM Masahiko Sawada
>  wrote:
> >
> >
> > I'm updating the patches and will submit them.
> 
> Attached updated version patches.
Thank you for sharing the patch v3.

Few minor comments.


(1) v03-0001, apply_error_callback function

-   /* append transaction information */
-   if (TransactionIdIsNormal(errarg->remote_xid))
+   if (errarg->rel == NULL)
{
-   appendStringInfo(&buf, _(" in transaction %u"), 
errarg->remote_xid);

Should write !errarg->rel ?

(2) v03-0002, doc/src/sgml/logical-replication.sgml


+   transaction that conflicts with the existing data.  When a conflict produces
+   an error, it is shown in the subscriber's server logs as follows:
+
+ERROR:  duplicate key value violates unique constraint "test_pkey"
+DETAIL:  Key (c)=(1) already exists.
+CONTEXT:  processing remote data during "INSERT" for replication target 
relation "public.test" in transaction 725 committed at LSN 0/14BFA88
+


We should update the CONTEXT message by using the v3-0001.

(3) v03-0002, doc/src/sgml/logical-replication.sgml


+   The LSN of the transaction that contains the change violating the 
constraint and
+   the replication origin name can be found from those outputs (LSN 0/14C0378 
and
+   replication origin pg_16395 in the above case).  To skip 
the
+   transaction, the subscription needs to be disabled temporarily by
+   ALTER SUBSCRIPTION ... DISABLE first. Then, the 
transaction
+   can be skipped by calling the 

The LSN(0/14C0378) is not same as the one in the above error context.
It's recommended to check LSNs directly written in the documentation.

(4) one confirmation

We don't have a TAP test of pg_replication_origin_advance()
for v3, that utilizes this new log in a logical replication setup.
This is because existing tests for this function (in test_decoding) is only for 
permission check
and argument validation, and we're just changing error message itself.
Is this correct ?


Best Regards,
Takamichi Osumi

false failure of test_docoding regression test

[Kyotaro Horiguchi]

Hello.

The CF-CI complained on one of my patch for seemingly a reason
unrelated to the patch.

https://cirrus-ci.com/task/5544213843542016?logs=test_world#L1666

> diff -U3 
> /tmp/cirrus-ci-build/contrib/test_decoding/expected/slot_creation_error.out 
> /tmp/cirrus-ci-build/contrib/test_decoding/output_iso/results/slot_creation_error.out
> --- 
> /tmp/cirrus-ci-build/contrib/test_decoding/expected/slot_creation_error.out   
> 2022-03-03 22:45:04.708072000 +
> +++ 
> /tmp/cirrus-ci-build/contrib/test_decoding/output_iso/results/slot_creation_error.out
>  2022-03-03 22:54:49.621351000 +
> @@ -96,13 +96,13 @@
>  t   
>  (1 row)
>  
> +step s1_c: COMMIT;
>  step s2_init: <... completed>
>  FATAL:  terminating connection due to administrator command
>  server closed the connection unexpectedly
>   This probably means the server terminated abnormally
>   before or while processing the request.
>  
> -step s1_c: COMMIT;
>  step s1_view_slot: 
>  SELECT slot_name, slot_type, active FROM pg_replication_slots WHERE 
> slot_name = 'slot_creation_error'

This comes from the permuattion 'permutation s1_b s1_xid s2_init
s1_terminate_s2 s1_c s1_view_slot'.  That means the process
termination by s1_terminate_s2 is a bit delayed until the next s1_c
ends.  So it is rare false failure but it is annoying enough on the
CI.  It seems to me we need to wait for process termination at the
time. postgres_fdw does that in regression test.

Thoughts?

Simliar use is found in temp-schema-cleanup. There's another possible
instability between s2_advisory and s2_check_schema but this change
alone reduces the chance for false failures.

The attached fixes the both points.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 82cdee92c3726a7f248849a310ec3f392ba02384 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Fri, 4 Mar 2022 11:03:12 +0900
Subject: [PATCH] Wait for process termination during isolation tests

slot_creation_error.spec and temp-schema-cleanup.spec used
pg_terminate_backend() without specifying timeout, thus there may be a
case of proceeding to the next step before the process actually
terminates then false failure.

Supply timeout to pg_terminate_backend() so that it waits for process
termination to avoid that failure mode.
---
 contrib/test_decoding/expected/slot_creation_error.out | 2 +-
 contrib/test_decoding/specs/slot_creation_error.spec   | 2 +-
 src/test/isolation/expected/temp-schema-cleanup.out| 2 +-
 src/test/isolation/specs/temp-schema-cleanup.spec  | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/test_decoding/expected/slot_creation_error.out b/contrib/test_decoding/expected/slot_creation_error.out
index 043bdae0a2..3707482cb8 100644
--- a/contrib/test_decoding/expected/slot_creation_error.out
+++ b/contrib/test_decoding/expected/slot_creation_error.out
@@ -87,7 +87,7 @@ step s2_init:
 SELECT 'init' FROM pg_create_logical_replication_slot('slot_creation_error', 'test_decoding');
  
 step s1_terminate_s2: 
-SELECT pg_terminate_backend(pid)
+SELECT pg_terminate_backend(pid, 18)
 FROM pg_stat_activity
 WHERE application_name = 'isolation/slot_creation_error/s2';
 
diff --git a/contrib/test_decoding/specs/slot_creation_error.spec b/contrib/test_decoding/specs/slot_creation_error.spec
index 6816696b9d..32161b9e7f 100644
--- a/contrib/test_decoding/specs/slot_creation_error.spec
+++ b/contrib/test_decoding/specs/slot_creation_error.spec
@@ -13,7 +13,7 @@ step s1_cancel_s2 {
 }
 
 step s1_terminate_s2 {
-SELECT pg_terminate_backend(pid)
+SELECT pg_terminate_backend(pid, 18)
 FROM pg_stat_activity
 WHERE application_name = 'isolation/slot_creation_error/s2';
 }
diff --git a/src/test/isolation/expected/temp-schema-cleanup.out b/src/test/isolation/expected/temp-schema-cleanup.out
index 35b91d9e45..cb4302739a 100644
--- a/src/test/isolation/expected/temp-schema-cleanup.out
+++ b/src/test/isolation/expected/temp-schema-cleanup.out
@@ -83,7 +83,7 @@ exec
 (1 row)
 
 step s1_exit: 
-SELECT pg_terminate_backend(pg_backend_pid());
+SELECT pg_terminate_backend(pg_backend_pid(), 18);
 
 FATAL:  terminating connection due to administrator command
 server closed the connection unexpectedly
diff --git a/src/test/isolation/specs/temp-schema-cleanup.spec b/src/test/isolation/specs/temp-schema-cleanup.spec
index a9417b7e90..f0d3928996 100644
--- a/src/test/isolation/specs/temp-schema-cleanup.spec
+++ b/src/test/isolation/specs/temp-schema-cleanup.spec
@@ -47,7 +47,7 @@ step s1_discard_temp {
 }
 
 step s1_exit {
-SELECT pg_terminate_backend(pg_backend_pid());
+SELECT pg_terminate_backend(pg_backend_pid(), 18);
 }
 
 
-- 
2.27.0