Re: GUC flags

[Michael Paquier]

On Sun, Nov 28, 2021 at 09:08:33PM -0600, Justin Pryzby wrote:
> This isn't flagged with GUC_EXPLAIN:
> enable_incremental_sort

Yeah, that's inconsistent.

> This isn't marked GUC_NOT_IN_SAMPLE, like all other DEVELOPER_OPTIONS:
> trace_recovery_messages

Indeed.

> I'm not sure jit_tuple_deforming should be marked GUC_NOT_IN_SAMPLE.
> I disable this one because it's slow for tables with many attributes.
> Same for jit_expressions ?

That would be consistent.  Both are not in postgresql.conf.sample.

> bgwriter_lru_maxpages should have GUC_UNIT_BLOCKS
>
> max_identifier_length should have BYTES (like log_parameter_max_length and
> track_activity_query_size).
>
> block_size and wal_block_size should say BYTES (like wal_segment_size)
> And all three descriptions should say ".. in [prefix]bytes" (but see below).

Okay for these.

> Maybe these should have COMPAT_OPTIONS_PREVIOUS:
> integer_datetimes
> ssl_renegotiation_limit

Hmm.  Okay as well for integer_datetimes.

> autovacuum_freeze_max_age has a comment about pg_resetwal which is obsolete
> since 74cf7d46a.
> 
> checkpoint_warning refers to "checkpoint segments", which is obsolete since
> 88e982302.

That's part of 0002.  That's a bit weird to use now, so I'd agree with
your suggestion to use "WAL segments" instead.

0001, to adjust the units, and 0003, to make the GUC descriptions less
unit-dependent, are good ideas.

-  gettext_noop("Use of huge pages on Linux or Windows."),
+  gettext_noop("Enable use of huge pages on Linux or Windows."),
This should be "Enables use of".

{"compute_query_id", PGC_SUSET, STATS_MONITORING,
-gettext_noop("Compute query identifiers."),
+gettext_noop("Enables in-core computation of a query identifier."),
This could just be "Computes"?

I am not completely sure that all the contents of 0002 are
improvements, but the suggestions done for huge_pages,
ssl_passphrase_command_supports_reload, checkpoint_warning and
commit_siblings seem fine to me.
--
Michael


signature.asc
Description: PGP signature

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Laurenz Albe]

On Sat, 2021-11-27 at 18:30 -0500, Tom Lane wrote:
> Trying to gather together the various issues mentioned on this thread,
> I see:
> 
> * Initial input lines that are blank (whitespace, maybe including a
> comment) are merged into the next command's history entry; but since
> said lines don't give rise to any text sent to the server, there's
> not really any reason why they couldn't be treated as text to be
> emitted to the history file immediately.  This is what Greg originally
> set out to change.  After my experiments mentioned above, I'm quite
> doubtful that his patch is correct in detail (I'm afraid that it
> probably emits stuff too soon in some cases), but it could likely be
> fixed if we could just get agreement that a change of that sort is OK.

For me, it is just a mild annoyance to have unrelated comments
preceding the query be part of the query's history file entry.
If that is difficult to improve, I can live with it the way it is.

> * It's not great that dash-dash comments aren't included in what we
> send to the server.  However, changing that is a lot trickier than
> it looks.  I think we want to continue suppressing comments that
> precede the query proper.  Including comments that are within the
> query text (ahead of the trailing semi) is not so hard, but comments
> following the semicolon look too much like comments-ahead-of-the-
> next-query.  Perhaps that issue should be left for another day ...
> although it does feel like it interacts with the first point.

If we treat double-dash comments differently from /* */ ones,
that is indeed odd.  I personally haven't been bothered by it, though.

> * Misbehavior of M-# was also mentioned.  Does anyone object to
> the draft patch I posted for that?

No, I think that is a clear improvement.

There was one other problem mentioned in the original mail, and that
seems to be the most serious one to me:

> psql
psql (14.1)
Type "help" for help.

test=> \set HISTCONTROL ignorespace   
test=>   -- line that starts with space
test=> SELECT 42;
 ?column? 
══
   42
(1 row)

Now that query is not added to the history file at all.

Yours,
Laurenz Albe

Fix typos

[qianglj.f...@fujitsu.com]

Hi,

I found several typos in comments and README. See patch attached.

Best regards,
Lingjie Qiang



0001-fix-typos.patch
Description: 0001-fix-typos.patch

RE: Data is copied twice when specifying both child and parent table in publication

[houzj.f...@fujitsu.com]

On Thursday, November 11, 2021 2:53 PM houzj.f...@fujitsu.com wrote:
> Attach the fix patch.
> 0001 fix data double publish(first issue in this thread)

In another thread[1], Amit L suggested that it'd be nice to add a testcase in
src/test/subscription/. So, attach a new version patch which add a testcase in
t/013_partition.pl.

[1] 
https://www.postgresql.org/message-id/CA%2BHiwqEjV%3D7iEW8hxnr73pWsDQuonDPLgsxXTYDQzDA7W9vrmw%40mail.gmail.com

Best regards,
Hou zj



v6-0001-Fix-double-publish-of-child-table-s-data.patch
Description: v6-0001-Fix-double-publish-of-child-table-s-data.patch

RE: [Proposal] Add foreign-server health checks infrastructure

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

Dear Zhihong,

Thank you for giving comments! I'll post new patches later.

> +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS()  
> (CheckingRemoteServersHoldoffCount++)
> 
> The macro contains only one operation. Can the macro be removed (with 
> `CheckingRemoteServersHoldoffCount++` inlined) ?

Hmm, these HOLD/RESUME macros are followed HOLD_INTERRUPUST() and 
HOLD_CANCEL_INTERRUPTS():

```
#define HOLD_INTERRUPTS()  (InterruptHoldoffCount++)

#define RESUME_INTERRUPTS() \
do { \
Assert(InterruptHoldoffCount > 0); \
InterruptHoldoffCount--; \
} while(0)

#define HOLD_CANCEL_INTERRUPTS()  (QueryCancelHoldoffCount++)

#define RESUME_CANCEL_INTERRUPTS() \
do { \
Assert(QueryCancelHoldoffCount > 0); \
QueryCancelHoldoffCount--; \
} while(0)

#define START_CRIT_SECTION()  (CritSectionCount++)

#define END_CRIT_SECTION() \
do { \
Assert(CritSectionCount > 0); \
CritSectionCount--; \
} while(0)
```

So I want to keep the current style. Could you tell me if you have any other 
reasons?

> +   if (CheckingRemoteServersTimeoutPending && 
> CheckingRemoteServersHoldoffCount != 0)
> +   {
> +   /*
> +* Skip checking foreign servers while reading messages.
> +*/
> +   InterruptPending = true;
> +   }
> +   else if (CheckingRemoteServersTimeoutPending)
> 
> Would the code be more readable if the above two if blocks be moved inside 
> one enclosing if block (factoring the common condition)?
> 
> +   if (CheckingRemoteServersTimeoutPending)

+1. Will fix.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Re: Windows: Wrong error message at connection termination

[Alexander Lakhin]

Hello Lars,
27.11.2021 14:39, Lars Kanis wrote:
>
> So I still think it's best to close the socket as proposed in the patch.
Please see also the previous discussion of the topic:
https://www.postgresql.org/message-id/flat/16678-253e48d34dc0c376%40postgresql.org

Best regards,
Alexander

[PATCH] DROP tab completion

[Ken Kato]

Hi hackers,

This time, I went through DROP tab completions
and noticed some tab completions missing for the following commands:
-DROP MATERIALIZED VIEW, DROP OWNED BY, DROP POLICY: missing 
[CASCADE|RESTRICT] at the end

-DROP TRANSFORM: no completions after TRANSFORM

I made a patch for this.

Best wishes,

--
Ken Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 630026da2f..2f412ca3db 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3322,12 +3322,16 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH("VIEW");
 	else if (Matches("DROP", "MATERIALIZED", "VIEW"))
 		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_matviews, NULL);
+	else if (Matches("DROP", "MATERIALIZED", "VIEW", MatchAny))
+		COMPLETE_WITH("CASCADE", "RESTRICT");
 
 	/* DROP OWNED BY */
 	else if (Matches("DROP", "OWNED"))
 		COMPLETE_WITH("BY");
 	else if (Matches("DROP", "OWNED", "BY"))
 		COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+	else if (Matches("DROP", "OWNED", "BY", MatchAny))
+		COMPLETE_WITH("CASCADE", "RESTRICT");
 
 	/* DROP TEXT SEARCH */
 	else if (Matches("DROP", "TEXT", "SEARCH"))
@@ -3368,6 +3372,8 @@ psql_completion(const char *text, int start, int end)
 		completion_info_charp = prev2_wd;
 		COMPLETE_WITH_QUERY(Query_for_list_of_tables_for_policy);
 	}
+	else if (Matches("DROP", "POLICY", MatchAny, "ON", MatchAny))
+		COMPLETE_WITH("CASCADE", "RESTRICT");
 
 	/* DROP RULE */
 	else if (Matches("DROP", "RULE", MatchAny))
@@ -3380,6 +3386,21 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches("DROP", "RULE", MatchAny, "ON", MatchAny))
 		COMPLETE_WITH("CASCADE", "RESTRICT");
 
+	/* DROP TRANSFORM */
+	else if (Matches("DROP", "TRANSFORM"))
+		COMPLETE_WITH("FOR");
+	else if (Matches("DROP", "TRANSFORM", "FOR"))
+		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_datatypes, NULL);
+	else if (Matches("DROP", "TRANSFORM", "FOR", MatchAny))
+		COMPLETE_WITH("LANGUAGE");
+	else if (Matches("DROP", "TRANSFORM", "FOR", MatchAny, "LANGUAGE"))
+	{
+		completion_info_charp = prev2_wd;
+		COMPLETE_WITH_QUERY(Query_for_list_of_languages);
+	}
+	else if (Matches("DROP", "TRANSFORM", "FOR", MatchAny, "LANGUAGE", MatchAny))
+		COMPLETE_WITH("CASCADE", "RESTRICT");
+
 /* EXECUTE */
 	else if (Matches("EXECUTE"))
 		COMPLETE_WITH_QUERY(Query_for_list_of_prepared_statements);

RE: pg_get_publication_tables() output duplicate relid

[houzj.f...@fujitsu.com]

On Wed, Nov 24, 2021 4:48 PM Amit Kapila 
> On Mon, Nov 22, 2021 at 12:55 PM Amit Langote 
> wrote:
> >
> > On Fri, Nov 19, 2021 at 2:28 PM Amit Kapila 
> wrote:
> > > On Fri, Nov 19, 2021 at 7:19 AM Amit Langote 
> wrote:
> > > >  As in,
> > > > do we know of any replication (initial/streaming) misbehavior
> > > > caused by the duplicate partition OIDs in this case or is the only
> > > > problem that pg_publication_tables output looks odd?
> > >
> > > The latter one but I think either we should document this or change
> > > it as we can't assume users will follow what subscriber-side code does.
> >
> > On second thought, I agree that de-duplicating partitions from this
> > view is an improvement.
> >
> 
> Fair enough. Hou-San, Can you please submit the updated patch after fixing
> any pending comments including the test case?

Attach the updated patch which address the comments so far.

The patch only adds a testcase in publication.sql because the duplicate
output doesn't cause unexpected behavior in pub-sub test.

Best regards,
Hou zj


v2-0001-de-duplicate-the-result-of-pg_get_publication_tables.patch
Description:  v2-0001-de-duplicate-the-result-of-pg_get_publication_tables.patch

RE: [Proposal] Add foreign-server health checks infrastructure

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

Dear Hackers,

I attached new version that is based from Zhihong's comments.
And I newly attached a doc patch. I think the infrastructure part is no longer 
WIP. 

Could you review them?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

<>


v04_0001_add_checking_infrastracture.patch
Description: v04_0001_add_checking_infrastracture.patch


v04_0002_add_doc.patch
Description: v04_0002_add_doc.patch

Re: [PATCH] buffile: ensure start offset is aligned with BLCKSZ

[Antonin Houska]

Sasasu  wrote:

> Hi hackers,
> 
> there are a very long discuss about TDE, and we agreed on that if the
> temporary file I/O can be aligned to some fixed size, it will be easier
> to use some kind of encryption algorithm.
> 
> discuss:
> https://www.postgresql.org/message-id/20211025155814.GD20998%40tamriel.snowman.net
> 
> This patch adjust file->curOffset and file->pos before the real IO to
> ensure the start offset is aligned.

Does this test really pass regression tests? In BufFileRead(), I would
understand if you did

+   file->pos = offsetInBlock;
+   file->curOffset -= offsetInBlock;

rather than

+   file->pos += offsetInBlock;
+   file->curOffset -= offsetInBlock;

Anyway, BufFileDumpBuffer() does not seem to enforce curOffset to end up at
block boundary, not to mention BufFileSeek().

When I was implementing this for our fork [1], I concluded that the encryption
code path is too specific, so I left the existing code for the unecrypted data
and added separate functions for the encrypted data.

One specific thing is that if you encrypt and write n bytes, but only need
part of it later, you need to read and decrypt exactly those n bytes anyway,
otherwise the decryption won't work. So I decided not only to keep curOffset
at BLCKSZ boundary, but also to read / write BLCKSZ bytes at a time. This also
makes sense if the scope of the initialization vector (IV) is BLCKSZ bytes.

Another problem is that you might want to store the IV somewhere in between
the data. In short, the encryption makes the buffered IO rather different and
the specific code should be kept aside, although the same API is used to
invoke it.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

[1] https://github.com/cybertec-postgresql/postgres/tree/PG_14_TDE_1_1

Re: row filtering for logical replication

[Amit Kapila]

On Mon, Nov 29, 2021 at 12:10 PM Greg Nancarrow  wrote:
>
> On Fri, Nov 26, 2021 at 12:40 AM houzj.f...@fujitsu.com
>  wrote:
> >
> > When researching and writing a top-up patch about this.
> > I found a possible issue which I'd like to confirm first.
> >
> > It's possible the table is published in two publications A and B, 
> > publication A
> > only publish "insert" , publication B publish "update". When UPDATE, both 
> > row
> > filter in A and B will be executed. Is this behavior expected?
> >
> > For example:
> >  Publication
> > create table tbl1 (a int primary key, b int);
> > create publication A for table tbl1 where (b<2) with(publish='insert');
> > create publication B for table tbl1 where (a>1) with(publish='update');
> >
> >  Subscription
> > create table tbl1 (a int primary key);
> > CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres host=localhost
> > port=1' PUBLICATION A,B;
> >
> >  Publication
> > update tbl1 set a = 2;
> >
> > The publication can be created, and when UPDATE, the rowfilter in A (b<2) 
> > will
> > also been executed but the column in it is not part of replica identity.
> > (I am not against this behavior just confirm)
> >
>
> There seems to be problems related to allowing the row filter to
> include columns that are not part of the replica identity (in the case
> of publish=insert).
> In your example scenario, the tbl1 WHERE clause "(b < 2)" for
> publication A, that publishes inserts only, causes a problem, because
> column "b" is not part of the replica identity.
> To see this, follow the simple example below:
> (and note, for the Subscription, the provided tbl1 definition has an
> error, it should also include the 2nd column "b int", same as in the
> publisher)
>
>  Publisher:
> INSERT INTO tbl1 VALUES (1,1);
> UPDATE tbl1 SET a = 2;
>
> Prior to the UPDATE above:
> On pub side, tbl1 contains (1,1).
> On sub side, tbl1 contains (1,1)
>
> After the above UPDATE:
> On pub side, tbl1 contains (2,1).
> On sub side, tbl1 contains (1,1), (2,1)
>
> So the UPDATE on the pub side has resulted in an INSERT of (2,1) on
> the sub side.
>
> This is because when (1,1) is UPDATEd to (2,1), it attempts to use the
> "insert" filter "(b<2)" to determine whether the old value had been
> inserted (published to subscriber), but finds there is no "b" value
> (because it only uses RI cols for UPDATE) and so has to assume the old
> tuple doesn't exist on the subscriber, hence the UPDATE ends up doing
> an INSERT.
> INow if the use of RI cols were enforced for the insert filter case,
> we'd properly know the answer as to whether the old row value had been
> published and it would have correctly performed an UPDATE instead of
> an INSERT in this case.
>

I don't think it is a good idea to combine the row-filter from the
publication that publishes just 'insert' with the row-filter that
publishes 'updates'. We shouldn't apply the 'insert' filter for
'update' and similarly for publication operations. We can combine the
filters when the published operations are the same. So, this means
that we might need to cache multiple row-filters but I think that is
better than having another restriction that publish operation 'insert'
should also honor RI columns restriction.

-- 
With Regards,
Amit Kapila.

Re: row filtering for logical replication

[Dilip Kumar]

On Mon, Nov 29, 2021 at 3:41 PM Amit Kapila  wrote:

> >  Publisher:
> > INSERT INTO tbl1 VALUES (1,1);
> > UPDATE tbl1 SET a = 2;
> >
> > Prior to the UPDATE above:
> > On pub side, tbl1 contains (1,1).
> > On sub side, tbl1 contains (1,1)
> >
> > After the above UPDATE:
> > On pub side, tbl1 contains (2,1).
> > On sub side, tbl1 contains (1,1), (2,1)
> >
> > So the UPDATE on the pub side has resulted in an INSERT of (2,1) on
> > the sub side.
> >
> > This is because when (1,1) is UPDATEd to (2,1), it attempts to use the
> > "insert" filter "(b<2)" to determine whether the old value had been
> > inserted (published to subscriber), but finds there is no "b" value
> > (because it only uses RI cols for UPDATE) and so has to assume the old
> > tuple doesn't exist on the subscriber, hence the UPDATE ends up doing
> > an INSERT.
> > INow if the use of RI cols were enforced for the insert filter case,
> > we'd properly know the answer as to whether the old row value had been
> > published and it would have correctly performed an UPDATE instead of
> > an INSERT in this case.
> >
>
> I don't think it is a good idea to combine the row-filter from the
> publication that publishes just 'insert' with the row-filter that
> publishes 'updates'. We shouldn't apply the 'insert' filter for
> 'update' and similarly for publication operations. We can combine the
> filters when the published operations are the same. So, this means
> that we might need to cache multiple row-filters but I think that is
> better than having another restriction that publish operation 'insert'
> should also honor RI columns restriction.

I am just wondering that if we don't combine filter in the above case
then what data we will send to the subscriber if the operation is
"UPDATE tbl1 SET a = 2, b=3", so in this case, we will apply only the
update filter i.e. a > 1 so as per that this will become the INSERT
operation because the old row was not passing the filter.  So now we
will insert a new row in the subscriber-side with value (2,3).  Looks
a bit odd to me that the value b=3 would have been rejected with the
direct insert but it is allowed due to indirect insert done by update.
Is this behavior looks odd only to me?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: row filtering for logical replication

[Amit Kapila]

On Mon, Nov 29, 2021 at 8:24 AM houzj.f...@fujitsu.com
 wrote:
>
> On Sun, Nov 28, 2021 3:18 PM Peter Smith  wrote:
> > On Fri, Nov 26, 2021 at 1:16 PM houzj.f...@fujitsu.com 
> >  wrote:
> > >
> > ...
> > > Based on this direction, I tried to write a top up POC patch(0005) which 
> > > I'd
> > > like to share.
> > >
> > > The top up patch mainly did the following things.
> > >
> > > * Move the row filter columns invalidation to CheckCmdReplicaIdentity, so
> > > that the invalidation is executed only when actual UPDATE or DELETE 
> > > executed on
> > > the published relation. It's consistent with the existing check about 
> > > replica
> > > identity.
> > >
> > > * Cache the results of the validation for row filter columns in relcache 
> > > to
> > > reduce the cost of the validation. It's safe because every operation that
> > > change the row filter and replica identity will invalidate the relcache.
> > >
> > > Also attach the v42 patch set to keep cfbot happy.
> >
> > Hi Hou-san.
> >
> > Thanks for providing your "top-up" 0005 patch!
> >
> > I suppose the goal will be to later merge this top-up with the current
> > 0002 validation patch, but in the meantime here are my review comments
> > for 0005.
>
> Thanks for the review and many valuable comments !
>
> > 8) src/backend/executor/execReplication.c - CheckCmdReplicaIdentity
> >
> > But which are the bad filter columns?
> >
> > Previously the Row Filter column validation gave errors for the
> > invalid filter column, but in this top-up patch there is no indication
> > which column or which filter or which publication was the bad one -
> > only that "something" bad was detected. IMO this might make it very
> > difficult for the user to know enough about the cause of the problem
> > to be able to fix the offending filter.
>
> If we want to report the invalid filter column, I can see two possibilities.
>
> 1) Instead of a bool flag, we cache a AttrNumber flag which indicates the
>invalid column number(0 means all valid). We can report it in the error
>message.
>
> 2) Everytime we decide to report an error, we traverse all the publications to
>find the invalid column again and report it.
>
> What do you think ?
>

I think we can probably give an error inside
RelationGetPublicationInfo(we can change the name of the function
based on changed functionality). Basically, if the row_filter is valid
then we can copy publication info from relcache and return it in
beginning, otherwise, allow it to check publications again. In error
cases, it shouldn't matter much to not use the cached information.
This is to some extent how the other parameters like rd_fkeyvalid and
rd_partcheckvalid works. One more thing, similar to some of the other
things isn't it better to manage pubactions and new bool flag directly
in relation instead of using PublicationInfo?

> 16) Tests... CREATE PUBLICATION succeeds
>
> > I have not yet reviewed any of the 0005 tests, but there was some big
> > behaviour difference that I noticed.
> >
> > I think now with the 0005 top-up patch the replica identify validation
> > is deferred to when UPDATE/DELETE is executed. I don’t know if this
> > will be very user friendly. It means now sometimes you can
> > successfully CREATE a PUBLICATION even though it will fail as soon as
> > you try to use it.
>
> I am not sure, the initial idea here is to make the check of replica identity
> consistent.
>
> Currently, if user create a publication which publish "update" but the 
> relation
> in the publication didn't mark as replica identity, then user can create the
> publication successfully. but the later UPDATE will report an error.
>

Yeah, I think giving an error on Update/Delete should be okay.

-- 
With Regards,
Amit Kapila.

Re: row filtering for logical replication

[Amit Kapila]

On Sun, Nov 28, 2021 at 12:48 PM Peter Smith  wrote:
>
> On Fri, Nov 26, 2021 at 1:16 PM houzj.f...@fujitsu.com
>  wrote:
> >
>
> 4) src/backend/catalog/pg_publication.c - check_rowfilter_replident
> +/*
> + * Check if all the columns used in the row-filter WHERE clause are part of
> + * REPLICA IDENTITY
> + */
> +bool
> +check_rowfilter_replident(Node *node, Bitmapset *bms_replident)
> +{
>
> IIUC here the false means "valid" and true means "invalid" which is
> counter-intuitive to me. So at least true/false meaning ought to be
> clarified in the function comment, and/or perhaps also rename the
> function so that the return meaning is more obvious.
>

+1 to rename the function in this case.

-- 
With Regards,
Amit Kapila.

Re: pg_upgrade and publication/subscription problem

[Marcos Pegoraro]

>
> On thinking about this point again, it is not clear to me why that
> would matter for this particular use case? Basically, when you create
> a new subscription, it should copy the entire existing data from the
> table directly and then will decode changes from WAL. So, I think in
> your case all the changes between pg_upgrade and now should be
> directly copied from tables, so probably older WAL won't be required.
>

Maybe you did not understand
Production server cannot stop while I  upgrade my subscriber server, so it
will be creating WAL continuously.

Subscriber server has trigger functions for auditing on all tables,
something like ...
insert into auditable(schemaname, tablename, primarykey, operation,
olddata, newdata) values(tg_table_schema, tg_table_name, getpk(new), tg_op,
row_to_json(old), row_to_json(new))

Then, all changes between pg_upgrade and now will not be inserted into
auditable.

Re: Commitfest 2021-11 Patch Triage - Part 1

[Marcos Pegoraro]

>
> > 2138: Incremental Materialized View Maintenance
>
> I've responded to it in the following thread, and described the recent
> discussions,
> current status, summary of IVM  feature and design, and past discussions.
>

IVM is a wonderful feature, but some features were omitted because of
its complexity, so maybe IVM will spend more time to completely solve what
it wants to do.
I did not understand, and didn´t find on docs, if a Materialized View is a
table, why I cannot change a specific record ?
Because if I have a way to create and refresh the entire view and update a
single record it would give me all power of IVM is trying to.

regards,
Marcos

Re: row filtering for logical replication

[Amit Kapila]

On Mon, Nov 29, 2021 at 4:36 PM Dilip Kumar  wrote:
>
> On Mon, Nov 29, 2021 at 3:41 PM Amit Kapila  wrote:
>
> > >  Publisher:
> > > INSERT INTO tbl1 VALUES (1,1);
> > > UPDATE tbl1 SET a = 2;
> > >
> > > Prior to the UPDATE above:
> > > On pub side, tbl1 contains (1,1).
> > > On sub side, tbl1 contains (1,1)
> > >
> > > After the above UPDATE:
> > > On pub side, tbl1 contains (2,1).
> > > On sub side, tbl1 contains (1,1), (2,1)
> > >
> > > So the UPDATE on the pub side has resulted in an INSERT of (2,1) on
> > > the sub side.
> > >
> > > This is because when (1,1) is UPDATEd to (2,1), it attempts to use the
> > > "insert" filter "(b<2)" to determine whether the old value had been
> > > inserted (published to subscriber), but finds there is no "b" value
> > > (because it only uses RI cols for UPDATE) and so has to assume the old
> > > tuple doesn't exist on the subscriber, hence the UPDATE ends up doing
> > > an INSERT.
> > > INow if the use of RI cols were enforced for the insert filter case,
> > > we'd properly know the answer as to whether the old row value had been
> > > published and it would have correctly performed an UPDATE instead of
> > > an INSERT in this case.
> > >
> >
> > I don't think it is a good idea to combine the row-filter from the
> > publication that publishes just 'insert' with the row-filter that
> > publishes 'updates'. We shouldn't apply the 'insert' filter for
> > 'update' and similarly for publication operations. We can combine the
> > filters when the published operations are the same. So, this means
> > that we might need to cache multiple row-filters but I think that is
> > better than having another restriction that publish operation 'insert'
> > should also honor RI columns restriction.
>
> I am just wondering that if we don't combine filter in the above case
> then what data we will send to the subscriber if the operation is
> "UPDATE tbl1 SET a = 2, b=3", so in this case, we will apply only the
> update filter i.e. a > 1 so as per that this will become the INSERT
> operation because the old row was not passing the filter.
>

If we want, I think for inserts (new row) we can consider the insert
filter as well but that makes it tricky to explain. I feel we can
change it later as well if there is a valid use case for this. What do
you think?

-- 
With Regards,
Amit Kapila.

Re: row filtering for logical replication

[Dilip Kumar]

On Mon, Nov 29, 2021 at 5:40 PM Amit Kapila  wrote:
>
> > > I don't think it is a good idea to combine the row-filter from the
> > > publication that publishes just 'insert' with the row-filter that
> > > publishes 'updates'. We shouldn't apply the 'insert' filter for
> > > 'update' and similarly for publication operations. We can combine the
> > > filters when the published operations are the same. So, this means
> > > that we might need to cache multiple row-filters but I think that is
> > > better than having another restriction that publish operation 'insert'
> > > should also honor RI columns restriction.
> >
> > I am just wondering that if we don't combine filter in the above case
> > then what data we will send to the subscriber if the operation is
> > "UPDATE tbl1 SET a = 2, b=3", so in this case, we will apply only the
> > update filter i.e. a > 1 so as per that this will become the INSERT
> > operation because the old row was not passing the filter.
> >
>
> If we want, I think for inserts (new row) we can consider the insert
> filter as well but that makes it tricky to explain. I feel we can
> change it later as well if there is a valid use case for this. What do
> you think?

Yeah, that makes sense.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: pg_upgrade and publication/subscription problem

[Amit Kapila]

On Mon, Nov 29, 2021 at 5:04 PM Marcos Pegoraro  wrote:
>>
>> On thinking about this point again, it is not clear to me why that
>> would matter for this particular use case? Basically, when you create
>> a new subscription, it should copy the entire existing data from the
>> table directly and then will decode changes from WAL. So, I think in
>> your case all the changes between pg_upgrade and now should be
>> directly copied from tables, so probably older WAL won't be required.
>
>
> Maybe you did not understand
>

Yeah, because some information like trigger functions was not there in
your previous emails.

-- 
With Regards,
Amit Kapila.

Re: Synchronizing slots from primary to standby

[Bharath Rupireddy]

On Mon, Nov 29, 2021 at 1:10 PM Dilip Kumar  wrote:
>
> On Mon, Nov 29, 2021 at 12:19 PM Bharath Rupireddy
>  wrote:
> >
> > On Mon, Nov 29, 2021 at 11:14 AM Dilip Kumar  wrote:
> > >
> > > On Mon, Nov 29, 2021 at 9:40 AM Bharath Rupireddy
> > >  wrote:
> > > >
> > > > On Mon, Nov 29, 2021 at 1:48 AM SATYANARAYANA NARLAPURAM
> > > >  wrote:
> > > > >
> > > > >> 3) Instead of the subscriber pulling the slot info, why can't the
> > > > >> publisher (via the walsender or a new bg worker maybe?) push the
> > > > >> latest slot info? I'm not sure we want to add more functionality to
> > > > >> the walsender, if yes, isn't it going to be much simpler?
> > > > >
> > > > > Standby pulling the information or at least making a first attempt to 
> > > > > connect to the  primary is a better design as primary doesn't need to 
> > > > > spend its cycles repeatedly connecting to an unreachable standby. In 
> > > > > fact, primary wouldn't even need to know the followers, for example 
> > > > > followers / log shipping standbys
> > > >
> > > > My idea was to let the existing walsender from the primary/publisher
> > > > to send the slot info (both logical and physical replication slots) to
> > > > the standby/subscriber, probably by piggybacking the slot info with
> > > > the WAL currently it sends. Having said that, I don't know the
> > > > feasibility of it. Anyways, I'm not in favour of having a new bg
> > > > worker to just ship the slot info. The standby/subscriber, while
> > > > making connection to primary/publisher, can choose to get the
> > > > replication slot info.
> > >
> > > I think it is possible that the standby is restoring the WAL directly
> > > from the archive location and there might not be any wal sender at
> > > time. So I think the idea of standby pulling the WAL looks better to
> > > me.
> >
> > My point was that why can't we let the walreceiver (of course users
> > can configure it on the standby/subscriber) to choose whether or not
> > to receive the replication (both physical and logical) slot info from
> > the primary/publisher and if yes, the walsender(on the
> > primary/publisher) sending it probably as a new WAL record or just
> > piggybacking the replication slot info with any of the existing WAL
> > records.
>
> Okay, I thought your point was that the primary pushing is better over
> standby pulling the slot info, but now it seems that you also agree
> that standby pulling is better right?  Now it appears your point is
> about whether we will use the same connection for pulling the slot
> information which we are using for streaming the data or any other
> connection?  I mean in this patch also we are creating a replication
> connection and pulling the slot information over there, just point is
> we are starting a separate worker for pulling the slot information,
> and I think that approach is better as this will not impact the
> performance of the other replication connection which we are using for
> communicating the data.

The easiest way to implement this feature so far, is to use a common
bg worker (as opposed to the bg worker proposed originally in this
thread which, IIUC, works for logical replication) running on standby
(in case of streaming replication with physical replication slots) or
subscriber (in case of logical replication with logical replication
slots) for getting both the physical and logical replication slots
info from the primary or publisher. This bg worker requires at least
two GUCs, 1) to enable/disable the worker 2) to define the slot sync
interval (the bg worker gets the slots info after every sync interval
of time).

Thoughts?

Regards,
Bharath Rupireddy.

Re: Switching XLog source from archive to streaming when primary available

[Dilip Kumar]

On Mon, Nov 29, 2021 at 1:30 AM SATYANARAYANA NARLAPURAM
 wrote:
>
> Hi Hackers,
>
> When the standby couldn't connect to the primary it switches the XLog source 
> from streaming to archive and continues in that state until it can get the 
> WAL from the archive location. On a server with high WAL activity, typically 
> getting the WAL from the archive is slower than streaming it from the primary 
> and couldn't exit from that state. This not only increases the lag on the 
> standby but also adversely impacts the primary as the WAL gets accumulated, 
> and vacuum is not able to collect the dead tuples. DBAs as a mitigation can 
> however remove/advance the slot or remove the restore_command on the standby 
> but this is a manual work I am trying to avoid. I would like to propose the 
> following, please let me know your thoughts.
>
> Automatically attempt to switch the source from Archive to streaming when the 
> primary_conninfo is set after replaying 'N' wal segment governed by the GUC 
> retry_primary_conn_after_wal_segments
> when  retry_primary_conn_after_wal_segments is set to -1 then the feature is 
> disabled
> When the retry attempt fails, then switch back to the archive

I think there is another thread [1] that is logically trying to solve
a similar issue, basically, in the main recovery apply loop is the
walreceiver does not exist then it is launching the walreceiver.
However, in that patch, it is not changing the current Xlog source but
I think that is not a good idea because with that it will restore from
the archive as well as stream from the primary so I have given that
review comment on that thread as well.  One big difference is that
patch is launching the walreceiver even if the WAL is locally
available and we don't really need more WAL but that is controlled by
a GUC.

[1] 
https://www.postgresql.org/message-id/CAKYtNApe05WmeRo92gTePEmhOM4myMpCK_%2BceSJtC7-AWLw1qw%40mail.gmail.com

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: [Proposal] Add foreign-server health checks infrastructure

[Zhihong Yu]

On Mon, Nov 29, 2021 at 12:51 AM kuroda.hay...@fujitsu.com <
kuroda.hay...@fujitsu.com> wrote:

> Dear Zhihong,
>
> Thank you for giving comments! I'll post new patches later.
>
> > +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS()
> (CheckingRemoteServersHoldoffCount++)
> >
> > The macro contains only one operation. Can the macro be removed (with
> `CheckingRemoteServersHoldoffCount++` inlined) ?
>
> Hmm, these HOLD/RESUME macros are followed HOLD_INTERRUPUST() and
> HOLD_CANCEL_INTERRUPTS():
>
> ```
> #define HOLD_INTERRUPTS()  (InterruptHoldoffCount++)
>
> #define RESUME_INTERRUPTS() \
> do { \
> Assert(InterruptHoldoffCount > 0); \
> InterruptHoldoffCount--; \
> } while(0)
>
> #define HOLD_CANCEL_INTERRUPTS()  (QueryCancelHoldoffCount++)
>
> #define RESUME_CANCEL_INTERRUPTS() \
> do { \
> Assert(QueryCancelHoldoffCount > 0); \
> QueryCancelHoldoffCount--; \
> } while(0)
>
> #define START_CRIT_SECTION()  (CritSectionCount++)
>
> #define END_CRIT_SECTION() \
> do { \
> Assert(CritSectionCount > 0); \
> CritSectionCount--; \
> } while(0)
> ```
>
> So I want to keep the current style. Could you tell me if you have any
> other reasons?
>
> > +   if (CheckingRemoteServersTimeoutPending &&
> CheckingRemoteServersHoldoffCount != 0)
> > +   {
> > +   /*
> > +* Skip checking foreign servers while reading messages.
> > +*/
> > +   InterruptPending = true;
> > +   }
> > +   else if (CheckingRemoteServersTimeoutPending)
> >
> > Would the code be more readable if the above two if blocks be moved
> inside one enclosing if block (factoring the common condition)?
> >
> > +   if (CheckingRemoteServersTimeoutPending)
>
> +1. Will fix.
>
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
>

Hi,

It is Okay to keep the macros.

Thanks

Re: Rationalizing declarations of src/common/ variables

[Robert Haas]

On Mon, Nov 29, 2021 at 12:57 AM Tom Lane  wrote:
> Here's a draft patch.  I'm not in love with the name "PGDLLIMPORT_FE"
> and would welcome better ideas.

What's the value of introducing PGDLLIMPORT_FE? I mean suppose we just
make PGDLLIMPORT expand to nothing in front-end code.

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

Re: pg_upgrade and publication/subscription problem

[Marcos Pegoraro]

Sorry, I didn´t explain exactly what I was doing, I just wrote "This
replication is a auditing database" on my second email.

Atenciosamente,




Em seg., 29 de nov. de 2021 às 09:20, Amit Kapila 
escreveu:

> On Mon, Nov 29, 2021 at 5:04 PM Marcos Pegoraro  wrote:
> >>
> >> On thinking about this point again, it is not clear to me why that
> >> would matter for this particular use case? Basically, when you create
> >> a new subscription, it should copy the entire existing data from the
> >> table directly and then will decode changes from WAL. So, I think in
> >> your case all the changes between pg_upgrade and now should be
> >> directly copied from tables, so probably older WAL won't be required.
> >
> >
> > Maybe you did not understand
> >
>
> Yeah, because some information like trigger functions was not there in
> your previous emails.
>
> --
> With Regards,
> Amit Kapila.
>

Lots of memory allocated when reassigning Large Objects

[Guillaume Lelarge]

Hi,

One of our customers had a very bad issue while trying to reassign objects
from user A to user B. He had a lot of them, and the backend got very
hungry for memory. It finally all went down when the linux kernel decided
to kill the backend (-9 of course).

I attach three shell scripts showing the issue. create.sh creates a
database and two users. Then it imports a million Large Objects in this new
database. There's no drop.sh as it is a simple "dropdb foodb".

session1_monitor.sh will start logging memory usage in the server log file
every second. So it needs v14, but our customer is in v11. While this
script runs, you can start session2_reindex.sh. This script will only run a
reassign from one user to another.

Here is what I get in the server log file:

$ grep "Grand total" 14.log
LOG:  Grand total: 15560832 bytes...
LOG:  Grand total: 68710528 bytes...
LOG:  Grand total: 119976064 bytes..
LOG:  Grand total: 171626624 bytes...
LOG:  Grand total: 224211072 bytes...
LOG:  Grand total: 276615296 bytes...
LOG:  Grand total: 325611648 bytes...
LOG:  Grand total: 378196096 bytes...
LOG:  Grand total: 429838464 bytes...
LOG:  Grand total: 481104000 bytes...

IOW, it's asking for at least 481MB to reassign 1 million empty LO. It
strikes me as odd.

FWIW, the biggest memory context is this one:

LOG:  level: 2; PortalContext: 479963904 total in 58590 blocks; 2662328
free (32567 chunks); 477301576 used: 

Memory is released at the end of the reassignment. So it's definitely not
leaked forever, but only during the operation, which looks like a missing
pfree (or something related). I've tried to find something like that in the
code somewhere, but to no avail. I'm pretty sure I missed something, which
is the reason for this email :)

Thanks.

Regards.

PS : we've found a workaround to make it work for our customer (executing
all the required ALTER LARGE OBJECT ... OWNER TO ...), but I'm still amazed
by this weird behaviour.


-- 
Guillaume.


create.sh
Description: application/shellscript


session2_reindex.sh
Description: application/shellscript


session1_monitor.sh
Description: application/shellscript

Re: Enforce work_mem per worker

[Arne Roland]

I did read parts of the last one back then. But thanks for the link, I plan to 
reread the thread as a whole.


>From what I can tell, the discussions here are the attempt by very smart 
>people to (at least partially) solve the problem of memory allocation (without 
>sacrificing to much on the runtime front). That problem is very hard.


What I am mostly trying to do, is to provide a reliable way of preventing the 
operational hazard of dealing with oom and alike, e.g. massive kernel buffer 
eviction. I don't want to touch the planning, which is always complex and tends 
to introduce weird side effects.


That way we can't hope to prevent the issue from occurring generally. I'm much 
more concerned with containing it, if it happens.


In the case that there is only a single pass, which tends to be the case for a 
lot of queries, my suggested approach would even help the offender.

But my main goal is something else. I can't explain my clients, why a chanced 
statistics due to autovacuum suddenly leads to oom. They would be right to 
question postgres qualification for any serious production system.


Regards

Arne

[PATCH] pg_statio_all_tables: several rows per table due to invalid TOAST index

[Andrei Zubkov]

Hi, hackers!

It seems we have a problem in pg_statio_all_tables view defenition.
According to the documentation and identification fields, this view
must have exact one row per a table.
The view definition contains an x.indexrelid as the last field in its
GROUP BY list:

<...>
GROUP BY c.oid, n.nspname, c.relname, t.oid, x.indexrelid

Which is the oid of a TOAST-index.

However it is possible that the TOAST table will have more than one
index. For example, this happens when REINDEX CONCURRENTLY operation
lefts an index in invalid state (indisvalid = false) due to some kind
of a failure. It's often sufficient to interrupt REINDEX CONCURRENTLY
operation right after start.

Such index will cause the second row to appear in a
pg_statio_all_tables view which obvious is unexpected behaviour.

Now we can have several regular indexes and several TOAST-indexes for
the same table. Statistics for the regular and TOAST indexes is to be
calculated the same way so I've decided to use a CTE here.

The proposed view definition follows:

CREATE VIEW pg_statio_all_tables AS
WITH indstat AS (
SELECT
indrelid,
sum(pg_stat_get_blocks_fetched(indexrelid) -
pg_stat_get_blocks_hit(indexrelid))::bigint
AS idx_blks_read,
sum(pg_stat_get_blocks_hit(indexrelid))::bigint
AS idx_blks_hit
FROM
pg_index
GROUP BY indrelid
)
SELECT
C.oid AS relid,
N.nspname AS schemaname,
C.relname AS relname,
pg_stat_get_blocks_fetched(C.oid) -
pg_stat_get_blocks_hit(C.oid) AS heap_blks_read,
pg_stat_get_blocks_hit(C.oid) AS heap_blks_hit,
I.idx_blks_read AS idx_blks_read,
I.idx_blks_hit AS idx_blks_hit,
pg_stat_get_blocks_fetched(T.oid) -
pg_stat_get_blocks_hit(T.oid) AS toast_blks_read,
pg_stat_get_blocks_hit(T.oid) AS toast_blks_hit,
X.idx_blks_read AS tidx_blks_read,
X.idx_blks_read AS tidx_blks_hit
FROM pg_class C LEFT JOIN
indstat I ON C.oid = I.indrelid LEFT JOIN
pg_class T ON C.reltoastrelid = T.oid LEFT JOIN
indstat X ON T.oid = X.indrelid
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE C.relkind IN ('r', 't', 'm');

Reported by Sergey Grinko.

Regards.
-- 
Andrei Zubkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

From ffde04cf285de32c7b8521c0aa9d0b36c1e8b7f7 Mon Sep 17 00:00:00 2001
From: Andrei Zubkov 
Date: Mon, 29 Nov 2021 16:33:34 +0300
Subject: [PATCH] pg_statio_all_tables: several rows per table due to invalid
 TOAST index

This patch changes definition of a pg_statio_all_tables view. More than one
index can exist on a TOAST table due to invalid indexes, which can appear due
to REINDEX CONCURRENTLY failure. Previous view definition in such case caused
several rows to appear in a view for a single table.

Reported by Sergey Grinko.
---
 src/backend/catalog/system_views.sql | 29 ++--
 src/test/regress/expected/rules.out  | 24 ++-
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index eb560955cd..84ec8e3989 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -708,6 +708,18 @@ CREATE VIEW pg_stat_xact_user_tables AS
   schemaname !~ '^pg_toast';
 
 CREATE VIEW pg_statio_all_tables AS
+WITH indstat AS (
+SELECT
+indrelid,
+sum(pg_stat_get_blocks_fetched(indexrelid) -
+pg_stat_get_blocks_hit(indexrelid))::bigint
+AS idx_blks_read,
+sum(pg_stat_get_blocks_hit(indexrelid))::bigint
+AS idx_blks_hit
+FROM
+pg_index
+GROUP BY indrelid
+)
 SELECT
 C.oid AS relid,
 N.nspname AS schemaname,
@@ -715,22 +727,19 @@ CREATE VIEW pg_statio_all_tables AS
 pg_stat_get_blocks_fetched(C.oid) -
 pg_stat_get_blocks_hit(C.oid) AS heap_blks_read,
 pg_stat_get_blocks_hit(C.oid) AS heap_blks_hit,
-sum(pg_stat_get_blocks_fetched(I.indexrelid) -
-pg_stat_get_blocks_hit(I.indexrelid))::bigint AS idx_blks_read,
-sum(pg_stat_get_blocks_hit(I.indexrelid))::bigint AS idx_blks_hit,
+I.idx_blks_read AS idx_blks_read,
+I.idx_blks_hit AS idx_blks_hit,
 pg_stat_get_blocks_fetched(T.oid) -
 pg_stat_get_blocks_hit(T.oid) AS toast_blks_read,
 pg_stat_get_blocks_hit(T.oid) AS toast_blks_hit,
-pg_stat_get_blocks_fetched(X.indexrelid) -
-pg_stat_get_blocks_hit(X.indexrelid) AS tidx_blks_read,
-pg_stat_get_blocks_hit(X.indexrelid) AS tidx_blks_hit
+

Re: A test for replay of regression tests

[Andrew Dunstan]

On 11/23/21 10:47, Andrew Dunstan wrote:
> On 11/23/21 04:07, Thomas Munro wrote:
>> On Wed, Oct 6, 2021 at 7:10 PM Thomas Munro  wrote:
>>> I wonder if for Windows we'd want to switch to real symlinks, since,
>>> as far as I know from some light reading, reparse points are converted
>>> to absolute paths on creation, so the pgdata directory would be less
>>> portable than it would be on POSIX systems.
>> I looked into that a bit, and it seems that the newer "real" symlinks
>> can't be created without admin privileges, unlike junction points, so
>> that wouldn't help.  I still need to figure out what this
>> junction-based patch set is doing wrong on Windows though trial-by-CI.
>> In the meantime, here is a rebase over recent changes to the Perl
>> testing APIs.
>
> More exactly you need to "Create Symbolic Links" privilege. see
> 
>
>
> I haven't yet worked out a way of granting that from the command line,
> but if it's working the buildfarm client (as of git tip) will use it on
> windows for the workdirs space saving feature.


Update:

There is a PowerShell module called Carbon which provides this and a
whole lot more. It can be installed in numerous ways, including via
Chocolatey. Here's what I am using:

choco install -y Carbon
Import-Module Carbon
Grant-CPrivilege -Identity myuser -Privilege SeCreateSymbolicLinkPrivilege

See  The command name I
used above is now the preferred spelling, although that's not reflected
on the manual page.


cheers


andrew

-- 

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

Re: Rationalizing declarations of src/common/ variables

[Tom Lane]

Robert Haas  writes:
> What's the value of introducing PGDLLIMPORT_FE? I mean suppose we just
> make PGDLLIMPORT expand to nothing in front-end code.

Hmm ... fair question.  It feels like that risks breaking something,
but offhand I can't see what, as long as we're certain that FRONTEND
is set correctly in every compile.

regards, tom lane

Re: Can I assume relation would not be invalid during from ExecutorRun to ExecutorEnd

[Tom Lane]

Andy Fan  writes:
> During my recent work,  I need some new stuff attached to Relation.  Rather
> than adding
> some new data structures,  I added it to Relation directly.  Like
> relation->balabala.  Then
> I initialize it during ExecutorRun,  like  table_tuple_insert.. and
> destroy it at ExecutorEnd.

This is not going to work, at least not if you expect that a relcache
reset would not preserve the data.  Also, what happens in the case of
nested executor runs touching the same relation?

Why do you think this ought to be in the relcache, and not in the
executor's rangetable-associated data structures?

regards, tom lane

Re: Rationalizing declarations of src/common/ variables

[Robert Haas]

On Mon, Nov 29, 2021 at 9:27 AM Tom Lane  wrote:
> Robert Haas  writes:
> > What's the value of introducing PGDLLIMPORT_FE? I mean suppose we just
> > make PGDLLIMPORT expand to nothing in front-end code.
>
> Hmm ... fair question.  It feels like that risks breaking something,
> but offhand I can't see what, as long as we're certain that FRONTEND
> is set correctly in every compile.

If it isn't, your way might go wrong too, since it depends on FRONTEND
being set correctly at least at the point when the PGDLLIMPORT_FE
macro is defined. But that is not to say that I think everything is in
great shape in this area. In a perfect world, I think the only
'#define FRONTEND 1' in the backend would be in postgres_fe.h, but we
have it in 5 other places too, 3 of which include a comment saying
that it's an "ugly hack". Until somebody cleans that mess up, we have
at least three cases to worry about: backend code that includes
"postgres.h", front code that includes "postgres-fe.h", and
frbontackend code that first does #define FRONTEND 1 and then includes
"postgres.h" anyway.

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

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Tom Lane]

Laurenz Albe  writes:
> There was one other problem mentioned in the original mail, and that
> seems to be the most serious one to me:
> [ HISTCONTROL behavior ]

The actual behavior of that option (which perhaps isn't adequately
documented) is that it suppresses a history entry if the first
character of the possibly-multi-line entry is a space.  It certainly
can't operate on a per-line basis, or you'd be likely to lose chunks
of a single SQL command, so I think that definition is fine as
it is (ignoring the whole question of whether the feature is sane
at all ... but if you don't think so, why would you use it?)

Greg's patch would fix this specifically by ensuring that the line
with the space and comment is treated as a separate history entry.
So I don't really see that as a separate bug.  Or, if you will,
the fact that people see it as a bug confirms that such a line
should be treated as a separate history entry.

regards, tom lane

Re: Can I assume relation would not be invalid during from ExecutorRun to ExecutorEnd

[Robert Haas]

On Mon, Nov 29, 2021 at 2:10 AM Andy Fan  wrote:
> 1.  During the ExecutorRun & ExecutorEnd,  the relcache will never by 
> invalidated, if not
> the old relation->balabala will be lost.  I assume this is correct since I 
> didn't see any places
> where we handle such changes in Executor code.

It's not correct. We accept invalidation messages in a number of
different code paths, including whenever we acquire a lock on a
relation. That doesn't typically happen in the middle of a query, but
that's just because most queries don't happen to do anything that
would make it happen. They can, though. For example, the query can
call a user-defined function that accesses a table not previously
touched by the transaction. Or a built-in function that does the same
thing, like table_to_xml().

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

Re: Rationalizing declarations of src/common/ variables

[Tom Lane]

Robert Haas  writes:
> On Mon, Nov 29, 2021 at 9:27 AM Tom Lane  wrote:
>> Robert Haas  writes:
>>> What's the value of introducing PGDLLIMPORT_FE? I mean suppose we just
>>> make PGDLLIMPORT expand to nothing in front-end code.

>> Hmm ... fair question.  It feels like that risks breaking something,
>> but offhand I can't see what, as long as we're certain that FRONTEND
>> is set correctly in every compile.

> If it isn't, your way might go wrong too, since it depends on FRONTEND
> being set correctly at least at the point when the PGDLLIMPORT_FE
> macro is defined.

Either of these ways would require that FRONTEND is already set correctly
when c.h is read.  But all of the hacks you mention do ensure that.

regards, tom lane

Re: Add connection active, idle time to pg_stat_activity

[Kuntal Ghosh]

On Fri, Oct 22, 2021 at 1:53 PM Rafia Sabih  wrote:
> To provide this information I was digging into how the statistics
> collector is working and found out there is already information like
> total time that a connection is active as well as idle computed in
> pgstat_report_activity[1]. Ideally, this would be the values we would
> like to see per process in pg_stat_activity.
It's definitely useful to know how much time a backend has spent for
query executions. Once you've this info, you can easily calculate the
idle time using this information: (now() - backend_start) -
active_time. But, I'm wondering why you need to distinguish between
idle and idle in transactions - what's the usage? Either the backend
is doing some work or it sits idle. Another useful information would
be when the last query execution was ended. From this information, you
can figure out whether a backend is idle for a long time since the
last execution and the execution time of the last query (query_end -
query_start).

You also need to update the documentation.

-- 
Thanks & Regards,
Kuntal Ghosh

Re: Catching missing Datum conversions

[Robert Haas]

On Fri, Jul 19, 2019 at 7:55 PM Thomas Munro  wrote:
> When reviewing a recent patch, I missed a place where Datum was being
> converted to another type implicitly (ie without going though a
> DatumGetXXX() macro).  Thanks to Jeff for fixing that (commit
> b538c90b), but I was curious to see if I could convince my compiler to
> tell me about that sort of thing.

This is a very easy mistake to make, so if you ever feel like
returning to this topic in earnest, I think it could be a worthwhile
expenditure of time.

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

Re: row filtering for logical replication

[Euler Taveira]

On Mon, Nov 29, 2021, at 7:11 AM, Amit Kapila wrote:
> I don't think it is a good idea to combine the row-filter from the
> publication that publishes just 'insert' with the row-filter that
> publishes 'updates'. We shouldn't apply the 'insert' filter for
> 'update' and similarly for publication operations. We can combine the
> filters when the published operations are the same. So, this means
> that we might need to cache multiple row-filters but I think that is
> better than having another restriction that publish operation 'insert'
> should also honor RI columns restriction.
That's exactly what I meant to say but apparently I didn't explain in details.
If a subscriber has multiple publications and a table is part of these
publications with different row filters, it should check the publication action
*before* including it in the row filter list. It means that an UPDATE operation
cannot apply a row filter that is part of a publication that has only INSERT as
an action. Having said that we cannot always combine multiple row filter
expressions into one. Instead, it should cache individual row filter expression
and apply the OR during the row filter execution (as I did in the initial
patches before this caching stuff). The other idea is to have multiple caches
for each action.  The main disadvantage of this approach is to create 4x
entries.

I'm experimenting the first approach that stores multiple row filters and its
publication action right now. Unfortunately we cannot use the
relentry->pubactions because it aggregates this information if you have
multiple entries. It seems a separate array should store this information that
will be used later while evaluating the row filter -- around
pgoutput_row_filter_exec_expr() call.


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

Re: Enforce work_mem per worker

[Justin Pryzby]

On Mon, Nov 29, 2021 at 02:01:35PM +, Arne Roland wrote:
> But my main goal is something else. I can't explain my clients, why a chanced 
> statistics due to autovacuum suddenly leads to oom. They would be right to 
> question postgres qualification for any serious production system.

What version postgres was that on ?

I realize it doesn't address your question, but since PG13, HashAggregate
respects work_mem.  Depending on the details of the query plan, that's arguably
a bigger problem than the absence of a global work_mem.  At least that one is
resolved.

-- 
Justin

Re: Lots of memory allocated when reassigning Large Objects

[Justin Pryzby]

On Mon, Nov 29, 2021 at 01:49:24PM +0100, Guillaume Lelarge wrote:
> One of our customers had a very bad issue while trying to reassign objects
> from user A to user B. He had a lot of them, and the backend got very
> hungry for memory. It finally all went down when the linux kernel decided
> to kill the backend (-9 of course).

> Memory is released at the end of the reassignment. So it's definitely not
> leaked forever, but only during the operation, which looks like a missing
> pfree (or something related). I've tried to find something like that in the
> code somewhere, but to no avail. I'm pretty sure I missed something, which
> is the reason for this email :)

I reproduced the issue like this.

psql postgres -c 'CREATE ROLE two WITH login superuser'
psql postgres two -c "SELECT lo_import('/dev/null') FROM 
generate_series(1,22111)" >/dev/null
psql postgres -c 'SET client_min_messages=debug; SET log_statement_stats=on;' 
-c 'begin; REASSIGN OWNED BY two TO pryzbyj; rollback;'

I didn't find the root problem, but was able to avoid the issue by creating a
new mem context.  I wonder if there are a bunch more issues like this.

unpatched:
!   33356 kB max resident size

patched:
!   21352 kB max resident size

diff --git a/src/backend/catalog/pg_shdepend.c 
b/src/backend/catalog/pg_shdepend.c
index 9ea42f805f..cbe7f04983 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -65,6 +65,7 @@
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/fmgroids.h"
+#include "utils/memutils.h"
 #include "utils/syscache.h"
 
 typedef enum
@@ -1497,6 +1498,11 @@ shdepReassignOwned(List *roleids, Oid newrole)
while ((tuple = systable_getnext(scan)) != NULL)
{
Form_pg_shdepend sdepForm = (Form_pg_shdepend) 
GETSTRUCT(tuple);
+   MemoryContext cxt, oldcxt;
+
+   cxt = AllocSetContextCreate(CurrentMemoryContext, 
"shdepReassignOwned cxt",
+   ALLOCSET_DEFAULT_SIZES);
+   oldcxt = MemoryContextSwitchTo(cxt);
 
/*
 * We only operate on shared objects and objects in the 
current
@@ -1598,8 +1604,12 @@ shdepReassignOwned(List *roleids, Oid newrole)
elog(ERROR, "unexpected classid %u", 
sdepForm->classid);
break;
}
+
/* Make sure the next iteration will see my changes */
CommandCounterIncrement();
+
+   MemoryContextSwitchTo(oldcxt);
+   MemoryContextDelete(cxt);
}
 
systable_endscan(scan);

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Alvaro Herrera]

On 2021-Nov-29, Tom Lane wrote:

> Greg's patch would fix this specifically by ensuring that the line
> with the space and comment is treated as a separate history entry.
> So I don't really see that as a separate bug.  Or, if you will,
> the fact that people see it as a bug confirms that such a line
> should be treated as a separate history entry.

I wonder if these things would be easier to deal with or more convenient
if we thought of -- as starting a line-scoped comment, and /* */ as
starting a query-scoped comment, and treat both types differently.  That
is, a -- comment would not be part of the subsequent command (and they
would become two separate history entries), but a /* */ comment would be
part of the command, and so both the comment and the query would be
saved as a single history entry.

So with ignorespace, then you can get either behavior:

 /* don't put neither comment nor command in history */
select 1;
and then nothing gets put in history; but if you do

 -- don't put this *comment* in history, but do put command
select 1;

then the comment is ignored, but the query does end up in history.


Perhaps one problem is how to behave with intra-query -- comments.
Surely they should not split the command in three parts, but I'm not
sure to what extent that is possible to implement.

This doesn't actually fix the issue that Greg was complaining about,
because his problem is precisely the -- comments.  But would Greg and
other users be satisfied if we made that distinction?

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Laurenz Albe]

On Mon, 2021-11-29 at 09:43 -0500, Tom Lane wrote:
> Laurenz Albe  writes:
> > There was one other problem mentioned in the original mail, and that
> > seems to be the most serious one to me:
> > [ HISTCONTROL behavior ]
> 
> The actual behavior of that option (which perhaps isn't adequately
> documented) is that it suppresses a history entry if the first
> character of the possibly-multi-line entry is a space.  It certainly
> can't operate on a per-line basis, or you'd be likely to lose chunks
> of a single SQL command, so I think that definition is fine as
> it is (ignoring the whole question of whether the feature is sane
> at all ... but if you don't think so, why would you use it?)
> 
> Greg's patch would fix this specifically by ensuring that the line
> with the space and comment is treated as a separate history entry.
> So I don't really see that as a separate bug.  Or, if you will,
> the fact that people see it as a bug confirms that such a line
> should be treated as a separate history entry.

Ah, yes.  You are right with both the explanation for the behavior
and stating that it points towards treating leading comments as
being seperate from the query.

And, thinking about HISTCONTROL, it does not seem sane in the
context of SQL, and I would never use it.

Yours,
Laurenz Albe

Re: Enforce work_mem per worker

[Arne Roland]

From: Justin Pryzby 
Sent: Monday, November 29, 2021 16:10
> On Mon, Nov 29, 2021 at 02:01:35PM +, Arne Roland wrote:
> > But my main goal is something else. I can't explain my clients, why a 
> > chanced statistics due to autovacuum suddenly leads to oom. They would be 
> > right to question postgres qualification for any serious production system.
>
> What version postgres was that on ?

It's pg13 and pg14 mostly. I have different servers with similar problems.

> I realize it doesn't address your question, but since PG13, HashAggregate
> respects work_mem.

I haven't run into issues with hash agg personally.

> Depending on the details of the query plan, that's arguably
> a bigger problem than the absence of a global work_mem.  At least that one is
> resolved.

I can go around to fix issues with plans. But plans are inherently unstable. 
And we can't have people becoming wary of autoanalyze.
Having a single wild plan bringing down a whole cluster is just madness.

There are bunch of different problems, that can occur. But where I stand this 
almost invalidates partition wise hash joins, because you'd generate one hash 
node per partition. But you can still have sorts with merge append, without 
partitionwise joins.
To quote your message from 2019:
> gracefully support[...ing] "thousands" of partitions
means using 1000 * work_mem?
Am I wrong here?

Regards
Arne

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Tom Lane]

Alvaro Herrera  writes:
> I wonder if these things would be easier to deal with or more convenient
> if we thought of -- as starting a line-scoped comment, and /* */ as
> starting a query-scoped comment, and treat both types differently.  That
> is, a -- comment would not be part of the subsequent command (and they
> would become two separate history entries), but a /* */ comment would be
> part of the command, and so both the comment and the query would be
> saved as a single history entry.

The hack I was fooling with yesterday would have had that effect,
although it was a consequence of the fact that I was too lazy
to parse slash-star comments ;-).  But basically what I was
trying to do was to force a line that was only whitespace
(possibly plus dash-dash comment) to be treated as a separate
history entry, while not suppressing dash-dash comments
altogether as the current code does.

regards, tom lane

Re: Non-superuser subscription owners

[Mark Dilger]

> On Nov 28, 2021, at 9:56 PM, Amit Kapila  wrote:
> 
> In ExecUpdate(), we convert Update to DELETE+INSERT when the
> partition constraint is failed whereas, on the subscriber-side, it
> will simply fail in this case. It is not clear to me how that is
> directly related to this patch but surely it will be a good
> improvement on its own and might help if that requires us to change
> some infrastructure here like hooking into executor at a higher level.

I would rather get a fix for non-superuser subscription owners committed than 
expand the scope of work and have this patch linger until the v16 development 
cycle.  This particular DELETE+INSERT problem sounds important but unrelated 
and out of scope.

> I agree that if we want to do all of this then that would require a
> lot of changes. However, giving an error for RLS-enabled tables might
> also be too restrictive. The few alternatives could be that (a) we
> allow subscription owners to be either have "bypassrls" attribute or
> they could be superusers. (b) don't allow initial table_sync for rls
> enabled tables. (c) evaluate/analyze what is required to allow Copy
> From to start respecting RLS policies. (d) reject replicating any
> changes to tables that have RLS enabled.
> 
> I see that you are favoring (d) which clearly has merits like lesser
> code/design change but not sure if that is the best way forward or we
> can do something better than that either by following one of (a), (b),
> (c), or something less restrictive than (d).

I was favoring option (d) only when RLS policies exist for one or more of the 
target relations.

Skipping the table_sync step while replicating tables that have RLS policies 
for subscriptions that are owned by users who lack bypassrls is interesting.  
If we make that work, it will be a more complete solution than option (d).

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

Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions

[Sascha Kuhl]

Buch (buchen sollst du suchen), Buchhaltung is great. Thanks for the
writing.

Stephen Frost  schrieb am Mo., 5. Aug. 2019, 21:02:

> Greetings,
>
> On Mon, Aug 5, 2019 at 14:43 Tom Lane  wrote:
>
>> Robert Haas  writes:
>> > On Mon, Aug 5, 2019 at 2:29 PM Tom Lane  wrote:
>> >> I think Stephen is not being unreasonable to suggest that we need some
>> >> documentation about what external tools may safely do to pg.auto.conf.
>> >> So somebody's got to write that.
>>
>> > I mean, really?  We're going to document that if you want to add a
>> > setting to the file, you can just append it, but that if you find
>> > yourself desirous of appending so many settings that the entire disk
>> > will fill up, you should maybe reconsider? Perhaps I'm being mean
>> > here, but that seems like it's straight out of the
>> > blinding-flashes-of-the-obvious department.
>>
>> I don't think we need to go on about it at great length, but it seems
>> to me that it'd be reasonable to point out that (a) you'd be well
>> advised not to touch the file while the postmaster is up, and (b)
>> last setting wins.  Those things are equally true of postgresql.conf
>> of course, but I don't recall whether they're already documented.
>
>
> Folks certainly modify postgresql.conf while the postmaster is running
> pretty routinely, and we expect them to which is why we have a reload
> option, so I don’t think we can say that the auto.conf and postgresql.conf
> are to be handled in the same way.
>
> Last setting wins, duplicates should be ignored and may be removed,
> comments should be ignored and may be removed, and appending to the file is
> acceptable for modifying a value.  I’m not sure how much we really document
> the structure of the file itself offhand- back when users were editing it
> we could probably be a bit more fast and loose with it, but now that we
> have different parts of the system modifying it along with external tools
> doing so, we should probably write it down a bit more clearly/precisely.
>
> I suspect the authors of pg_conftool would appreciate that too, so they
> could make sure that they aren’t doing anything unexpected or incorrect.
>
> Thanks,
>
> Stephen
>
>>

Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions

[Sascha Kuhl]

To give you another thanks: IT is compatible with discapacity. Great

Sascha Kuhl  schrieb am Mo., 29. Nov. 2021, 17:39:

> Buch (buchen sollst du suchen), Buchhaltung is great. Thanks for the
> writing.
>
> Stephen Frost  schrieb am Mo., 5. Aug. 2019, 21:02:
>
>> Greetings,
>>
>> On Mon, Aug 5, 2019 at 14:43 Tom Lane  wrote:
>>
>>> Robert Haas  writes:
>>> > On Mon, Aug 5, 2019 at 2:29 PM Tom Lane  wrote:
>>> >> I think Stephen is not being unreasonable to suggest that we need some
>>> >> documentation about what external tools may safely do to pg.auto.conf.
>>> >> So somebody's got to write that.
>>>
>>> > I mean, really?  We're going to document that if you want to add a
>>> > setting to the file, you can just append it, but that if you find
>>> > yourself desirous of appending so many settings that the entire disk
>>> > will fill up, you should maybe reconsider? Perhaps I'm being mean
>>> > here, but that seems like it's straight out of the
>>> > blinding-flashes-of-the-obvious department.
>>>
>>> I don't think we need to go on about it at great length, but it seems
>>> to me that it'd be reasonable to point out that (a) you'd be well
>>> advised not to touch the file while the postmaster is up, and (b)
>>> last setting wins.  Those things are equally true of postgresql.conf
>>> of course, but I don't recall whether they're already documented.
>>
>>
>> Folks certainly modify postgresql.conf while the postmaster is running
>> pretty routinely, and we expect them to which is why we have a reload
>> option, so I don’t think we can say that the auto.conf and postgresql.conf
>> are to be handled in the same way.
>>
>> Last setting wins, duplicates should be ignored and may be removed,
>> comments should be ignored and may be removed, and appending to the file is
>> acceptable for modifying a value.  I’m not sure how much we really document
>> the structure of the file itself offhand- back when users were editing it
>> we could probably be a bit more fast and loose with it, but now that we
>> have different parts of the system modifying it along with external tools
>> doing so, we should probably write it down a bit more clearly/precisely.
>>
>> I suspect the authors of pg_conftool would appreciate that too, so they
>> could make sure that they aren’t doing anything unexpected or incorrect.
>>
>> Thanks,
>>
>> Stephen
>>
>>>

Re: Rationalizing declarations of src/common/ variables

[Robert Haas]

On Mon, Nov 29, 2021 at 10:03 AM Tom Lane  wrote:
> Either of these ways would require that FRONTEND is already set correctly
> when c.h is read.  But all of the hacks you mention do ensure that.

Yeah. Are you aware of any other, worse hacks?

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

Re: Rationalizing declarations of src/common/ variables

[Tom Lane]

Robert Haas  writes:
> On Mon, Nov 29, 2021 at 10:03 AM Tom Lane  wrote:
>> Either of these ways would require that FRONTEND is already set correctly
>> when c.h is read.  But all of the hacks you mention do ensure that.

> Yeah. Are you aware of any other, worse hacks?

Worse than which?  Anyway, I pushed a patch based on your suggestion;
we'll soon see if the Windows BF members like it.

regards, tom lane

Re: Rename dead_tuples to dead_items in vacuumlazy.c

[Peter Geoghegan]

On Wed, Nov 24, 2021 at 4:48 AM Masahiko Sawada  wrote:
> The patch renames dead tuples to dead items at some places and  to
> dead TIDs at some places.

> I think it's more consistent if we change it to one side. I prefer "dead 
> items".

I just pushed a version of the patch that still uses both terms when
talking about dead_items. But the final commit actually makes it clear
why, in comments above the LVDeadItems struct itself: LVDeadItems is
used by both index vacuuming and heap vacuuming.

Thanks
-- 
Peter Geoghegan

Re: XMAX_LOCK_ONLY and XMAX_COMMITTED (fk/multixact code)

[Bossart, Nathan]

On 11/25/21, 9:16 AM, "Mark Dilger"  wrote:
>> On Nov 24, 2021, at 12:53 PM, Bossart, Nathan  wrote:
>>
>> Another option we might consider is only checking for the
>> HEAP_XMAX_LOCK_ONLY bit instead of everything in
>> HEAP_XMAX_IS_LOCKED_ONLY.  IIUC everything else is only expected to
>> happen for upgrades from v9.2 or earlier, so it might be pretty rare
>> at this point.  Otherwise, I'll extract the exact bit pattern for the
>> error message as you suggest.
>
>I would prefer to detect and report any "can't happen" bit patterns without 
>regard for how likely the pattern may be.  The difficulty is in proving that a 
>bit pattern is disallowed.  Just because you can't find a code path in the 
>current code base that would create a pattern doesn't mean it won't have 
>legitimately been created by some past release or upgrade path.  As such, any 
>prohibitions explicitly in the backend, such as Asserts around a condition, 
>are really valuable.  You can know that the pattern is disallowed, since the 
>server would Assert on it if encountered.
>
> Aside from that, I don't really buy the argument that databases upgraded from 
> v9.2 or earlier are rare.  Even if servers *running* v9.2 or earlier are (or 
> become) rare, servers initialized that far back which have been upgraded one 
> or more times since then may be common.

Okay, I'll do it that way in the next revision.

Nathan

Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)

[Andres Freund]

Hi,

Thank for all the work on this topic - I'd somehow accidentally marked this
thread as read when coming back from vacation...

Greetings,

Andres Freund

Re: Non-superuser subscription owners

[Jeff Davis]

On Mon, 2021-11-29 at 08:26 -0800, Mark Dilger wrote:
> > On Nov 28, 2021, at 9:56 PM, Amit Kapila 
> > wrote:
> > 
> > In ExecUpdate(), we convert Update to DELETE+INSERT when the
> > partition constraint is failed whereas, on the subscriber-side, it
> > will simply fail in this case.

Thank you, yes, that's the more important case.

> This particular DELETE+INSERT problem sounds important but unrelated
> and out of scope.

+1

> > I agree that if we want to do all of this then that would require a
> > lot of changes. However, giving an error for RLS-enabled tables
> > might
> > also be too restrictive. The few alternatives could be that (a) we
> > allow subscription owners to be either have "bypassrls" attribute
> > or
> > they could be superusers. (b) don't allow initial table_sync for
> > rls
> > enabled tables. (c) evaluate/analyze what is required to allow Copy
> > From to start respecting RLS policies. (d) reject replicating any
> > changes to tables that have RLS enabled.

Maybe a combination?

Allow subscriptions with copy_data=true iff the subscription owner is
bypassrls or superuser. And then enforce RLS+WCO during
insert/update/delete.

I don't think it's a big change (correct me if I'm wrong), and it
allows good functionality now, and room to improve in the future if we
want to bring in more of ExecInsert into logical replication.

Regards,
Jeff Davis

Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file

[Bossart, Nathan]

On 11/26/21, 7:33 AM, "Tom Lane"  wrote:
> Michael Paquier  writes:
>> On Thu, Nov 25, 2021 at 06:19:03PM -0800, SATYANARAYANA NARLAPURAM wrote:
>>> If we are keeping it then why not make it better?
>
>> Well, non-exclusive backups are better by design in many aspects, so I
>> don't quite see the point in spending time on something that has more
>> limitations than what's already in place.
>
> IMO the main reason for keeping it is backwards compatibility for users
> who have a satisfactory backup arrangement using it.  That same argument
> implies that we shouldn't change how it works (at least, not very much).

The issues with exclusive backups seem to be fairly well-documented
(e.g., c900c15), but perhaps there should also be a note in the
"Backup Control Functions" table [0].

Nathan

[0] 
https://www.postgresql.org/docs/devel/functions-admin.html#FUNCTIONS-ADMIN-BACKUP

Re: Lots of memory allocated when reassigning Large Objects

[Tom Lane]

Justin Pryzby  writes:
> I reproduced the issue like this.

> psql postgres -c 'CREATE ROLE two WITH login superuser'
> psql postgres two -c "SELECT lo_import('/dev/null') FROM 
> generate_series(1,22111)" >/dev/null
> psql postgres -c 'SET client_min_messages=debug; SET log_statement_stats=on;' 
> -c 'begin; REASSIGN OWNED BY two TO pryzbyj; rollback;'

Confirmed here, although I needed to use a lot more than 22K large objects
to see a big leak.

> I didn't find the root problem, but was able to avoid the issue by creating a
> new mem context.  I wonder if there are a bunch more issues like this.

I poked into it with valgrind, and identified the major leak as being
stuff that is allocated by ExecOpenIndices and not freed by
ExecCloseIndices.  The latter knows it's leaking:

/*
 * XXX should free indexInfo array here too?  Currently we assume that
 * such stuff will be cleaned up automatically in FreeExecutorState.
 */

On the whole, I'd characterize this as DDL code using pieces of the
executor without satisfying the executor's expectations as to environment
--- specifically, that it'll be run in a memory context that doesn't
outlive executor shutdown.  Normally, any one DDL operation does a limited
number of catalog updates so that small per-update leaks don't cost that
much ... but REASSIGN OWNED creates a loop that can invoke ALTER OWNER
many times.

I think your idea of creating a short-lived context is about right.
Another idea we could consider is to do that within CatalogTupleUpdate;
but I'm not sure that the cost/benefit ratio would be good for most
operations.  Anyway I think ALTER OWNER has other leaks outside the
index-update operations, so we'd still need to do this within
REASSIGN OWNED's loop.

DROP OWNED BY likely has similar issues.

regards, tom lane

Re: Lots of memory allocated when reassigning Large Objects

[Guillaume Lelarge]

Le lun. 29 nov. 2021 à 19:40, Tom Lane  a écrit :

> Justin Pryzby  writes:
> > I reproduced the issue like this.
>
> > psql postgres -c 'CREATE ROLE two WITH login superuser'
> > psql postgres two -c "SELECT lo_import('/dev/null') FROM
> generate_series(1,22111)" >/dev/null
> > psql postgres -c 'SET client_min_messages=debug; SET
> log_statement_stats=on;' -c 'begin; REASSIGN OWNED BY two TO pryzbyj;
> rollback;'
>
> Confirmed here, although I needed to use a lot more than 22K large objects
> to see a big leak.
>
>
So do I.

> I didn't find the root problem, but was able to avoid the issue by
> creating a
> > new mem context.  I wonder if there are a bunch more issues like this.
>
> I poked into it with valgrind, and identified the major leak as being
> stuff that is allocated by ExecOpenIndices and not freed by
> ExecCloseIndices.  The latter knows it's leaking:
>
> /*
>  * XXX should free indexInfo array here too?  Currently we assume
> that
>  * such stuff will be cleaned up automatically in
> FreeExecutorState.
>  */
>
> On the whole, I'd characterize this as DDL code using pieces of the
> executor without satisfying the executor's expectations as to environment
> --- specifically, that it'll be run in a memory context that doesn't
> outlive executor shutdown.  Normally, any one DDL operation does a limited
> number of catalog updates so that small per-update leaks don't cost that
> much ... but REASSIGN OWNED creates a loop that can invoke ALTER OWNER
> many times.
>
> I think your idea of creating a short-lived context is about right.
> Another idea we could consider is to do that within CatalogTupleUpdate;
> but I'm not sure that the cost/benefit ratio would be good for most
> operations.  Anyway I think ALTER OWNER has other leaks outside the
> index-update operations, so we'd still need to do this within
> REASSIGN OWNED's loop.
>
>
I've tried Justin's patch but it didn't help with my memory allocation
issue. FWIW, I attach the patch I used in v14.

DROP OWNED BY likely has similar issues.
>
>
Didn't try it, but it wouldn't be a surprise.


-- 
Guillaume.
diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index d89a9fa2bf..68dad27afb 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -65,6 +65,7 @@
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/fmgroids.h"
+#include "utils/memutils.h"
 #include "utils/syscache.h"
 
 typedef enum
@@ -1549,6 +1550,10 @@ shdepReassignOwned(List *roleids, Oid newrole)
 		while ((tuple = systable_getnext(scan)) != NULL)
 		{
 			Form_pg_shdepend sdepForm = (Form_pg_shdepend) GETSTRUCT(tuple);
+			MemoryContext cxt, oldcxt;
+
+			cxt = AllocSetContextCreate(CurrentMemoryContext, "shdepReassignOwned cxt", ALLOCSET_DEFAULT_SIZES);
+			oldcxt = MemoryContextSwitchTo(cxt);
 
 			/*
 			 * We only operate on shared objects and objects in the current
@@ -1656,6 +1661,9 @@ shdepReassignOwned(List *roleids, Oid newrole)
 			}
 			/* Make sure the next iteration will see my changes */
 			CommandCounterIncrement();
+
+			MemoryContextSwitchTo(oldcxt);
+			MemoryContextDelete(cxt);
 		}
 
 		systable_endscan(scan);

Re: Windows: Wrong error message at connection termination

[Tom Lane]

Alexander Lakhin  writes:
> 27.11.2021 14:39, Lars Kanis wrote:
>> So I still think it's best to close the socket as proposed in the patch.

> Please see also the previous discussion of the topic:
> https://www.postgresql.org/message-id/flat/16678-253e48d34dc0c376%40postgresql.org

Hm, yeah, that discussion seems to have slipped through the cracks.
Not sure why it didn't end up in pushing something.

After re-reading that thread and re-studying relevant Windows
documentation [1][2], I think the main open question is whether
we need to issue shutdown() or not, and if so, whether to use
SD_BOTH or just SD_SEND.  I'm inclined to prefer not calling
shutdown(), because [1] is self-contradictory as to whether it
can block, and [2] is pretty explicit that it's not necessary.

regards, tom lane

[1] 
https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-shutdown
[2] 
https://docs.microsoft.com/en-us/windows/win32/winsock/graceful-shutdown-linger-options-and-socket-closure-2

Re: Non-superuser subscription owners

[Jeff Davis]

On Mon, 2021-11-29 at 09:43 +0530, Amit Kapila wrote:
> The first reason is that way it would be consistent with what we can
> see while doing the operations from the backend.

Logical replication is not interactive, so it doesn't seem quite the
same.

If you have a long running INSERT INTO SELECT or COPY FROM, the
permission checks just happen at the beginning. As a user, it wouldn't
surprise me if logical replication was similar.

> operation. Another reason is to make behavior predictable as users
> can
> always expect when exactly the privilege change will be reflected and
> it won't depend on the number of changes in the transaction.

This patch does detect ownership changes more quickly (at the
transaction boundary) than the current code (only when it reloads for
some other reason). Transaction boundary seems like a reasonable time
to detect the change to me.

Detecting faster might be nice, but I don't have a strong opinion about
it and I don't see why it necessarily needs to happen before this patch
goes in.

Also, do you think the cost of doing maybe_reread_subscription() per-
tuple instead of per-transaction would be detectable? If we lock
ourselves into semantics that detect changes quickly, it will be harder
to optimize the per-tuple path later.

Regards,
Jeff Davis

Re: Lots of memory allocated when reassigning Large Objects

[Tom Lane]

Guillaume Lelarge  writes:
> I've tried Justin's patch but it didn't help with my memory allocation
> issue. FWIW, I attach the patch I used in v14.

[ looks closer ... ]  Ah, that patch is a bit buggy: it fails to do the
right thing in the cases where the loop does a "continue".  The attached
revision seems to behave properly.

I still see a small leakage, which I think is due to accumulation of
pending sinval messages for the catalog updates.  I'm curious whether
that's big enough to be a problem for Guillaume's use case.  (We've
speculated before about bounding the memory used for pending sinval
in favor of just issuing a cache reset when the list would be too
big.  But nobody's done anything about it, suggesting that people
seldom have a problem in practice.)

>> DROP OWNED BY likely has similar issues.

> Didn't try it, but it wouldn't be a surprise.

I tried just changing the REASSIGN to a DROP in Justin's example,
and immediately hit

ERROR:  out of shared memory
HINT:  You might need to increase max_locks_per_transaction.

thanks to the per-object locks we try to acquire.  So I'm not
sure that the DROP case can reach an interesting amount of
local memory leaked before it runs out of lock-table space.

regards, tom lane

diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index 9ea42f805f..d7f0708396 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -65,6 +65,7 @@
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/fmgroids.h"
+#include "utils/memutils.h"
 #include "utils/syscache.h"
 
 typedef enum
@@ -1497,6 +1498,7 @@ shdepReassignOwned(List *roleids, Oid newrole)
 		while ((tuple = systable_getnext(scan)) != NULL)
 		{
 			Form_pg_shdepend sdepForm = (Form_pg_shdepend) GETSTRUCT(tuple);
+			MemoryContext cxt, oldcxt;
 
 			/*
 			 * We only operate on shared objects and objects in the current
@@ -1510,6 +1512,11 @@ shdepReassignOwned(List *roleids, Oid newrole)
 			if (sdepForm->deptype != SHARED_DEPENDENCY_OWNER)
 continue;
 
+			cxt = AllocSetContextCreate(CurrentMemoryContext,
+		"shdepReassignOwned",
+		ALLOCSET_DEFAULT_SIZES);
+			oldcxt = MemoryContextSwitchTo(cxt);
+
 			/* Issue the appropriate ALTER OWNER call */
 			switch (sdepForm->classid)
 			{
@@ -1598,6 +1605,10 @@ shdepReassignOwned(List *roleids, Oid newrole)
 	elog(ERROR, "unexpected classid %u", sdepForm->classid);
 	break;
 			}
+
+			MemoryContextSwitchTo(oldcxt);
+			MemoryContextDelete(cxt);
+
 			/* Make sure the next iteration will see my changes */
 			CommandCounterIncrement();
 		}

Re: Lots of memory allocated when reassigning Large Objects

[Guillaume Lelarge]

Le lun. 29 nov. 2021 à 20:39, Tom Lane  a écrit :

> Guillaume Lelarge  writes:
> > I've tried Justin's patch but it didn't help with my memory allocation
> > issue. FWIW, I attach the patch I used in v14.
>
> [ looks closer ... ]  Ah, that patch is a bit buggy: it fails to do the
> right thing in the cases where the loop does a "continue".  The attached
> revision seems to behave properly.
>
> I still see a small leakage, which I think is due to accumulation of
> pending sinval messages for the catalog updates.  I'm curious whether
> that's big enough to be a problem for Guillaume's use case.  (We've
> speculated before about bounding the memory used for pending sinval
> in favor of just issuing a cache reset when the list would be too
> big.  But nobody's done anything about it, suggesting that people
> seldom have a problem in practice.)
>
>
I've tried your patch with my test case. It still uses a lot of memory.
Actually even more.

I have this with the log_statement_stats:

1185072 kB max resident size

And I have this with the log-memory-contexts function:

LOG:  Grand total: 1007796352 bytes in 320 blocks; 3453512 free (627
chunks); 1004342840 used

Contrary to Justin's patch, the shdepReassignOwned doesn't seem to be used.
I don't get any shdepReassignOwned line in the log file. I tried multiple
times to avoid any mistake on my part, but got same result.

>> DROP OWNED BY likely has similar issues.
>
> > Didn't try it, but it wouldn't be a surprise.
>
> I tried just changing the REASSIGN to a DROP in Justin's example,
> and immediately hit
>
> ERROR:  out of shared memory
> HINT:  You might need to increase max_locks_per_transaction.
>
> thanks to the per-object locks we try to acquire.  So I'm not
> sure that the DROP case can reach an interesting amount of
> local memory leaked before it runs out of lock-table space.
>
>
I've hit the same issue when I tried my ALTER LARGE OBJECT workaround in
one transaction.


-- 
Guillaume.

Re: Commitfest 2021-11 Patch Triage - Part 2

[Rémi Lapeyre]

> 
> 2780: Allow COPY "text" to output a header and add header matching mode to 
> COPY
> FROM
> ===
> The original patch was rejected for lacking matching, but it has since been
> addressed in an updated patch which has seen a lot of review.  Reading the
> thread it seems this should really be in Ready for Committer.

I don’t think there is anything more that needs to be done for this patch. I 
have been checking that it apply cleanly and that the tests still pass but it 
has already multiple rounds of reviews and is probably ready.

Best regards,
Rémi

Re: improve CREATE EXTENSION error message

[Daniel Gustafsson]

> On 29 Nov 2021, at 20:54, Bossart, Nathan  wrote:
> 
> Hi hackers,
> 
> Currently, if you attempt to use CREATE EXTENSION for an extension
> that is not installed, you'll see something like the following:
> 
>postgres=# CREATE EXTENSION does_not_exist;
>ERROR:  could not open extension control file 
> "/usr/local/pgsql/share/extension/does_not_exist.control": No such file or 
> directory
> 
> I suspect this ERROR message is confusing for novice users, so perhaps
> we should add a HINT.  With the attached patch, you'd see the
> following:
> 
>postgres=# CREATE EXTENSION does_not_exist;
>ERROR:  could not open extension control file 
> "/usr/local/pgsql/share/extension/does_not_exist.control": No such file or 
> directory
>HINT:  This typically indicates that the specified extension is not 
> installed on the system.
> 
> Thoughts?

I haven't given the suggested wording too much thought, but in general that
sounds like a good idea.

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

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Tom Lane]

After some further hackery, here's a set of patches that I think
might be acceptable.  They're actually fairly independent, although
they touch different aspects of the same behavior.

0001 gets rid of psqlscan.l's habit of suppressing dash-dash comments,
but only once we have collected some non-whitespace query input.
The upshot of this is that dash-dash comments will get sent to the
server as long as they are within the query proper, that is after the
first non-whitespace token and before the ending semicolon.  Comments
that are between queries are still suppressed, because not doing that
seems to result in far too much behavioral change.  As it stands,
though, there are just a few regression test result changes.

0002 is a simplified version of Greg's patch.  I think we only need
to look at the state of the query_buf to see if any input has been
collected in order to determine if we are within or between queries.
I'd originally thought this'd need to be a lot more complicated,
but as long as psqlscan.l continues to drop pre-query comments,
this seems to be enough.

0003 is the same patch I posted before to adjust M-# behavior.

regards, tom lane

diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index b52d187722..e0abe34bb6 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -101,7 +101,7 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
 --+---+--
  PREPARE pgss_test (int) AS SELECT $1, $2 LIMIT $3| 1 |1
  SELECT $1   +| 4 |4
- +|   | 
+   -- multiline  +|   | 
AS "text"  |   | 
  SELECT $1 + $2   | 2 |2
  SELECT $1 + $2 + $3 AS "add" | 3 |3
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index 0fab48a382..325b440a12 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -376,12 +376,11 @@ other			.
 	/*
 	 * Note that the whitespace rule includes both true
 	 * whitespace and single-line ("--" style) comments.
-	 * We suppress whitespace at the start of the query
-	 * buffer.  We also suppress all single-line comments,
-	 * which is pretty dubious but is the historical
-	 * behavior.
+	 * We suppress whitespace until we have collected some
+	 * non-whitespace data.  (This interacts with some
+	 * decisions in MainLoop(); see there for details.)
 	 */
-	if (!(output_buf->len == 0 || yytext[0] == '-'))
+	if (output_buf->len > 0)
 		ECHO;
 }
 
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 5949996ebc..be5fa5727d 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1905,14 +1905,14 @@ from generate_series(1,5) x,
  (values (0::float8),(0.1),(0.25),(0.4),(0.5),(0.6),(0.75),(0.9),(1)) v(p)
 group by p order by p;
 ERROR:  sum is not an ordered-set aggregate, so it cannot have WITHIN GROUP
-LINE 1: select p, sum() within group (order by x::float8)  
+LINE 1: select p, sum() within group (order by x::float8)  -- error
   ^
 select p, percentile_cont(p,p)  -- error
 from generate_series(1,5) x,
  (values (0::float8),(0.1),(0.25),(0.4),(0.5),(0.6),(0.75),(0.9),(1)) v(p)
 group by p order by p;
 ERROR:  WITHIN GROUP is required for ordered-set aggregate percentile_cont
-LINE 1: select p, percentile_cont(p,p)  
+LINE 1: select p, percentile_cont(p,p)  -- error
   ^
 select percentile_cont(0.5) within group (order by b) from aggtest;
  percentile_cont  
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index c2e5676196..cb9373227d 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -928,7 +928,7 @@ CREATE TRIGGER gtest2a BEFORE INSERT OR UPDATE ON gtest26
   WHEN (NEW.b < 0)  -- error
   EXECUTE PROCEDURE gtest_trigger_func();
 ERROR:  BEFORE trigger's WHEN condition cannot reference NEW generated columns
-LINE 3:   WHEN (NEW.b < 0)  
+LINE 3:   WHEN (NEW.b < 0)  -- error
 ^
 DETAIL:  Column "b" is a generated column.
 CREATE TRIGGER gtest2b BEFORE INSERT OR UPDATE ON gtest26
@@ -936,7 +936,7 @@ CREATE TRIGGER gtest2b BEFORE INSERT OR UPDATE ON gtest26
   WHEN (NEW.* IS NOT NULL)  -- error
   EXECUTE PROCEDURE gtest_trigger_func

Re: improve CREATE EXTENSION error message

[Tom Lane]

"Bossart, Nathan"  writes:
> Currently, if you attempt to use CREATE EXTENSION for an extension
> that is not installed, you'll see something like the following:

> postgres=# CREATE EXTENSION does_not_exist;
> ERROR:  could not open extension control file 
> "/usr/local/pgsql/share/extension/does_not_exist.control": No such file or 
> directory

> I suspect this ERROR message is confusing for novice users, so perhaps
> we should add a HINT.

If we issue the hint only for errno == ENOENT, I think we could be
less wishy-washy (and if it's not ENOENT, the hint is likely
inappropriate anyway).  I'm thinking something more like

HINT:  This means the extension is not installed on the system.

I'm not quite satisfied with the "on the system" wording, but I'm
not sure of what would be better.  I agree that we can't just say
"is not installed", because people will confuse that with whether
it is installed within the database.

regards, tom lane

Re: SSL Tests for sslinfo extension

[Daniel Gustafsson]

> On 27 Nov 2021, at 20:27, Tom Lane  wrote:

> I don't have any problem with this structurally, but I do have a
> few nitpicks:

Thanks for reviewing!

> * I think the error message added in 0001 should complain about
> missing password "encryption" not "encoding", no?

Doh, of course.

> * 0002 hasn't been updated for the great PostgresNode renaming.

Fixed.

> * 0002 needs to extend src/test/ssl/README to mention that
> "make installcheck" requires having installed contrib/sslinfo,
> analogous to similar comments in (eg) src/test/recovery/README.

Good point, I copied over the wording from recovery/README and adapted for SSL
since I think it was well written as is. (Consistency is also a good benefit.)

> * 0002 writes a temporary file in the source tree.  This is bad;
> for one thing I bet it fails under VPATH, but in any case there
> is no reason to risk it.  Put it in the tmp_check directory instead
> (cf temp kdc files in src/test/kerberos/t/001_auth.pl).  That's
> safer and you needn't worry about cleaning it up.

Fixed, and see below.

> * Hmm ... now I notice that you borrowed the key-file-copying logic
> from the 001 and 002 tests, but it's just as bad practice there.
> We should fix them too.

Well spotted, I hadn't thought about that but in hindsight it's quite obviously
bad.  I've done this in a 0003 patch in this series which also comes with the
IMO benefit of a tighter coupling between the key filename used in the test
with what's in the repo by removing the _tmp suffix.  To avoid concatenating
with the long tmp_check path variable everywhere, I went with a lookup HASH to
make it easier on the eye and harder to mess up should we change tmp path at
some point.  There might be ways which are more like modern Perl, but I wasn't
able to think of one off the bat.

> * I ran a code-coverage check and it shows that this doesn't test
> ssl_issuer_field() or any of the following functions in sslinfo.c.
> I think at least ssl_extension_info() is complicated enough to
> deserve a test.

Agreed.  The attached v3 covers the issuer and extension function to at least
some degree.  In order to reliably test the extension I added a new cert with a
CA extension.

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



v3-0001-Extend-configure_test_server_for_ssl-to-add-exten.patch
Description: Binary data


v3-0002-Add-tests-for-sslinfo.patch
Description: Binary data


v3-0003-Use-test-specific-temp-path-for-keys-during-SSL-t.patch
Description: Binary data

Re: improve CREATE EXTENSION error message

[Bossart, Nathan]

On 11/29/21, 12:33 PM, "Daniel Gustafsson"  wrote:
> I haven't given the suggested wording too much thought, but in general that
> sounds like a good idea.

Thanks.  I'm flexible with the wording.

Nathan

Re: Lots of memory allocated when reassigning Large Objects

[Tom Lane]

Guillaume Lelarge  writes:
> Le lun. 29 nov. 2021 à 20:39, Tom Lane  a écrit :
>> [ looks closer ... ]  Ah, that patch is a bit buggy: it fails to do the
>> right thing in the cases where the loop does a "continue".  The attached
>> revision seems to behave properly.

> I've tried your patch with my test case. It still uses a lot of memory.
> Actually even more.

Hmm ... I tried it with your test case, and I see the backend completing
the query without going beyond 190MB used (which is mostly shared memory).
Without the patch it blows past that point very quickly indeed.

I'm checking it in HEAD though; perhaps there's something else wrong
in the back branches?

regards, tom lane

Re: improve CREATE EXTENSION error message

[Daniel Gustafsson]

> On 29 Nov 2021, at 22:02, Tom Lane  wrote:
> 
> "Bossart, Nathan"  writes:
>> Currently, if you attempt to use CREATE EXTENSION for an extension
>> that is not installed, you'll see something like the following:
> 
>>postgres=# CREATE EXTENSION does_not_exist;
>>ERROR:  could not open extension control file 
>> "/usr/local/pgsql/share/extension/does_not_exist.control": No such file or 
>> directory
> 
>> I suspect this ERROR message is confusing for novice users, so perhaps
>> we should add a HINT.
> 
> If we issue the hint only for errno == ENOENT, I think we could be
> less wishy-washy (and if it's not ENOENT, the hint is likely
> inappropriate anyway).  I'm thinking something more like
> 
> HINT:  This means the extension is not installed on the system.
> 
> I'm not quite satisfied with the "on the system" wording, but I'm
> not sure of what would be better.  I agree that we can't just say
> "is not installed", because people will confuse that with whether
> it is installed within the database.

That's a good point, the hint is targeting users who might not even know that
an extension needs to be physically and separately installed on the machine
before it can be installed in their database; so maybe using "installed" here
isn't entirely helpful at all.  That being said I'm at a loss for a more
suitable word, "available" perhaps?

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

Re: improve CREATE EXTENSION error message

[Bossart, Nathan]

On 11/29/21, 1:03 PM, "Tom Lane"  wrote:
> If we issue the hint only for errno == ENOENT, I think we could be
> less wishy-washy (and if it's not ENOENT, the hint is likely
> inappropriate anyway).  I'm thinking something more like
>
> HINT:  This means the extension is not installed on the system.

Good idea.

> I'm not quite satisfied with the "on the system" wording, but I'm
> not sure of what would be better.  I agree that we can't just say
> "is not installed", because people will confuse that with whether
> it is installed within the database.

Right.  The only other idea I have at the moment is to say something
like

This means the extension is not available[ on the system].

I don't know whether that is actually any less confusing, though.

Nathan

Re: Lots of memory allocated when reassigning Large Objects

[Guillaume Lelarge]

Le lun. 29 nov. 2021 à 22:27, Tom Lane  a écrit :

> Guillaume Lelarge  writes:
> > Le lun. 29 nov. 2021 à 20:39, Tom Lane  a écrit :
> >> [ looks closer ... ]  Ah, that patch is a bit buggy: it fails to do the
> >> right thing in the cases where the loop does a "continue".  The attached
> >> revision seems to behave properly.
>
> > I've tried your patch with my test case. It still uses a lot of memory.
> > Actually even more.
>
> Hmm ... I tried it with your test case, and I see the backend completing
> the query without going beyond 190MB used (which is mostly shared memory).
> Without the patch it blows past that point very quickly indeed.
>
> I'm checking it in HEAD though; perhaps there's something else wrong
> in the back branches?
>
>
That's also what I was thinking. I was only trying with v14. I just checked
with v15devel, and your patch works alright. So there must be something
else with back branches.


-- 
Guillaume.

Re: improve CREATE EXTENSION error message

[Chapman Flack]

On 11/29/21 16:31, Daniel Gustafsson wrote:
> That's a good point, the hint is targeting users who might not even know that
> an extension needs to be physically and separately installed on the machine
> before it can be installed in their database; so maybe using "installed" here
> isn't entirely helpful at all.  That being said I'm at a loss for a more
> suitable word, "available" perhaps?

Maybe a larger break with the "This means the extension something something"
formulation, and more on the lines of

HINT:  an extension must first be present (for example, installed with a
 package manager) on the system where PostgreSQL is running.

Regards,
-Chap

Re: improve CREATE EXTENSION error message

[Bossart, Nathan]

On 11/29/21, 1:38 PM, "Chapman Flack"  wrote:
> On 11/29/21 16:31, Daniel Gustafsson wrote:
>> That's a good point, the hint is targeting users who might not even know that
>> an extension needs to be physically and separately installed on the machine
>> before it can be installed in their database; so maybe using "installed" here
>> isn't entirely helpful at all.  That being said I'm at a loss for a more
>> suitable word, "available" perhaps?
>
> Maybe a larger break with the "This means the extension something something"
> formulation, and more on the lines of
>
> HINT:  an extension must first be present (for example, installed with a
>  package manager) on the system where PostgreSQL is running.

I like this idea.  I can do it this way in the next revision if others
agree.

Nathan

Re: Lots of memory allocated when reassigning Large Objects

[Tom Lane]

Guillaume Lelarge  writes:
> Le lun. 29 nov. 2021 à 22:27, Tom Lane  a écrit :
>> I'm checking it in HEAD though; perhaps there's something else wrong
>> in the back branches?

> That's also what I was thinking. I was only trying with v14. I just checked
> with v15devel, and your patch works alright. So there must be something
> else with back branches.

Thanks for confirming, I'll dig into it later.

regards, tom lane

Re: improve CREATE EXTENSION error message

[Daniel Gustafsson]

> On 29 Nov 2021, at 22:47, Bossart, Nathan  wrote:
> 
> On 11/29/21, 1:38 PM, "Chapman Flack"  wrote:
>> On 11/29/21 16:31, Daniel Gustafsson wrote:
>>> That's a good point, the hint is targeting users who might not even know 
>>> that
>>> an extension needs to be physically and separately installed on the machine
>>> before it can be installed in their database; so maybe using "installed" 
>>> here
>>> isn't entirely helpful at all.  That being said I'm at a loss for a more
>>> suitable word, "available" perhaps?
>> 
>> Maybe a larger break with the "This means the extension something something"
>> formulation, and more on the lines of
>> 
>> HINT:  an extension must first be present (for example, installed with a
>> package manager) on the system where PostgreSQL is running.
> 
> I like this idea.  I can do it this way in the next revision if others
> agree.

I think taking it in this direction has merits.

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

Re: Lots of memory allocated when reassigning Large Objects

[Guillaume Lelarge]

Le lun. 29 nov. 2021 à 22:47, Tom Lane  a écrit :

> Guillaume Lelarge  writes:
> > Le lun. 29 nov. 2021 à 22:27, Tom Lane  a écrit :
> >> I'm checking it in HEAD though; perhaps there's something else wrong
> >> in the back branches?
>
> > That's also what I was thinking. I was only trying with v14. I just
> checked
> > with v15devel, and your patch works alright. So there must be something
> > else with back branches.
>
> Thanks for confirming, I'll dig into it later.
>
>
Thanks a lot.


-- 
Guillaume.

Re: improve CREATE EXTENSION error message

[Tom Lane]

Daniel Gustafsson  writes:
>> On 29 Nov 2021, at 22:47, Bossart, Nathan  wrote:
>> On 11/29/21, 1:38 PM, "Chapman Flack"  wrote:
>>> Maybe a larger break with the "This means the extension something something"
>>> formulation, and more on the lines of
>>> HINT:  an extension must first be present (for example, installed with a
>>> package manager) on the system where PostgreSQL is running.

>> I like this idea.  I can do it this way in the next revision if others
>> agree.

> I think taking it in this direction has merits.

I think "The extension must ..." would read better, otherwise +1.

I don't especially like intertwining the hint choice with the existing
special case for per-version files.  Our usual style for conditional
hints can be found in places like sysv_shmem.c, and following that
would lead to a patch roughly like

if ((file = AllocateFile(filename, "r")) == NULL)
{
+   int ext_errno = errno;
+
if (version && errno == ENOENT)
{
/* no auxiliary file for this version */
pfree(filename);
return;
}
ereport(ERROR,
(errcode_for_file_access(),
 errmsg("could not open extension control file \"%s\": %m",
-   filename)));
+   filename),
+(ext_errno == ENOENT) ? errhint("...") : 0));
}

regards, tom lane

Re: improve CREATE EXTENSION error message

[Bossart, Nathan]

On 11/29/21, 2:13 PM, "Bossart, Nathan"  wrote:
> Alright, here's v3.  In this version, I actually removed the message
> about the control file entirely, so now the error message looks like
> this:
>
> postgres=# CREATE EXTENSION does_not_exist;
> ERROR:  extension "does_not_exist" is not available
> DETAIL:  The extension must first be installed on the system where 
> PostgreSQL is running.
> HINT:  The pg_available_extensions view lists the extensions that are 
> available for installation.
>
> I can add the control file part back if we think it's necessary.

Hm.  I should probably adjust the hint to avoid confusion from
"installed on the system" and "available for installation."  Maybe
something like

The pg_available_extensions view lists the available extensions.

Nathan

Re: improve CREATE EXTENSION error message

[Chapman Flack]

On 11/29/21 17:03, Tom Lane wrote:
> I think "The extension must ..." would read better, otherwise +1.

I'm not strongly invested either way, but I'll see if I can get at why
I used 'an' ...

Hints are hinty. We can give them, and they're helpful, because there
are certain situations that we know are very likely to be what's behind
certain errors. ENOENT on the control file? Yeah, probably means the
extension needs to be installed. In somebody's specific case, though,
it could mean most of the extension is there but the other sysadmin
overnight fat-fingered an rm command and has been spending the morning
idly wondering why the file he /meant/ to remove is still there. Or a bit
flipped in an inode and a directory became a file. (That happened to me on
a production system once; the directory was /usr. That'll mess stuff up.)

So, in my view, a hint doesn't need to sound omniscient, or as if it
somehow knows precisely what happened in your case. It's enough (maybe
better, even?) if a hint reads like a hint, a general statement that
you may ponder for a moment and then think "yeah, that sounds like it's
probably what I needed to know."

Regards,
-Chap

Re: improve CREATE EXTENSION error message

[Chapman Flack]

On 11/29/21 17:13, Bossart, Nathan wrote:
> postgres=# CREATE EXTENSION does_not_exist;
> ERROR:  extension "does_not_exist" is not available
> DETAIL:  The extension must first be installed on the system where 
> PostgreSQL is running.
> HINT:  The pg_available_extensions view lists the extensions that are 
> available for installation.

Messages crossed ...

If it were me, I would combine that DETAIL and HINT as one larger HINT,
and use DETAIL for specific details about what actually happened (such
as the exact filename sought and the %m).

The need for those details doesn't go away; they're still what you need
when what went wrong is some other freak occurrence the hint doesn't
explain.

Regards,
-Chap

Re: improve CREATE EXTENSION error message

[Tom Lane]

"Bossart, Nathan"  writes:
> Alright, here's v3.  In this version, I actually removed the message
> about the control file entirely, so now the error message looks like
> this:

> postgres=# CREATE EXTENSION does_not_exist;
> ERROR:  extension "does_not_exist" is not available
> DETAIL:  The extension must first be installed on the system where 
> PostgreSQL is running.
> HINT:  The pg_available_extensions view lists the extensions that are 
> available for installation.

I don't think that HINT is useful at all, and I agree with Chapman
that we should still show the filename we tried to look up,
just in case there's a path problem or the like.

regards, tom lane

Re: improve CREATE EXTENSION error message

[Bossart, Nathan]

On 11/29/21, 2:32 PM, "Chapman Flack"  wrote:
> If it were me, I would combine that DETAIL and HINT as one larger HINT,
> and use DETAIL for specific details about what actually happened (such
> as the exact filename sought and the %m).
>
> The need for those details doesn't go away; they're still what you need
> when what went wrong is some other freak occurrence the hint doesn't
> explain.

How's this?

postgres=# CREATE EXTENSION does_not_exist;
ERROR:  extension "does_not_exist" is not available
DETAIL:  Extension control file 
"/usr/local/pgsql/share/extension/does_not_exist.control" does not exist.
HINT:  The extension must first be installed on the system where 
PostgreSQL is running.  The pg_available_extensions view lists the available 
extensions.

Nathan

Re: SSL Tests for sslinfo extension

[Tom Lane]

Daniel Gustafsson  writes:
> Agreed.  The attached v3 covers the issuer and extension function to at least
> some degree.  In order to reliably test the extension I added a new cert with 
> a
> CA extension.

I have two remaining trivial nitpicks, for which I attach an 0004
delta patch: the README change was fat-fingered slightly, and some
of the commentary about the key file seems now obsolete.

Otherwise I think it's good to go, so I marked it RFC.

regards, tom lane

diff --git a/src/test/ssl/README b/src/test/ssl/README
index ca30f9329a..7e60700652 100644
--- a/src/test/ssl/README
+++ b/src/test/ssl/README
@@ -12,14 +12,12 @@ TCP connections on localhost. Any user on the same host is able to
 log in to the test server while the tests are running. Do not run this
 suite on a multi-user system where you don't trust all local users!
 
-NOTE: You must have given the --enable-tap-tests argument to configure.
-Also, to use "make installcheck", you must have built and installed
-contrib/sslinfo in addition to the core code.
-
 Running the tests
 =
 
 NOTE: You must have given the --enable-tap-tests argument to configure.
+Also, to use "make installcheck", you must have built and installed
+contrib/sslinfo in addition to the core code.
 
 Run
 make check
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 61b117e6c2..cf2e8dde0f 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -37,9 +37,6 @@ my $common_connstr;
 
 # The client's private key must not be world-readable, so take a copy
 # of the key stored in the code tree and update its permissions.
-#
-# This changes ssl/client.key to ssl/client_tmp.key etc for the rest
-# of the tests.
 my $client_tmp_key = "${PostgreSQL::Test::Utils::tmp_check}/client_ext.key";
 copy("ssl/client_ext.key", $client_tmp_key)
   or die "couldn't copy ssl/client_ext.key to $client_tmp_key for permissions change: $!";

Re: SSL Tests for sslinfo extension

[Daniel Gustafsson]

> On 29 Nov 2021, at 23:50, Tom Lane  wrote:
> 
> Daniel Gustafsson  writes:
>> Agreed.  The attached v3 covers the issuer and extension function to at least
>> some degree.  In order to reliably test the extension I added a new cert 
>> with a
>> CA extension.
> 
> I have two remaining trivial nitpicks, for which I attach an 0004
> delta patch: the README change was fat-fingered slightly, and some
> of the commentary about the key file seems now obsolete.

Ah yes, thanks.

> Otherwise I think it's good to go, so I marked it RFC.

Great!  I'll take another look over it tomorrow and will go ahead with it then.

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

Re: Lots of memory allocated when reassigning Large Objects

[Tom Lane]

Guillaume Lelarge  writes:
> Le lun. 29 nov. 2021 à 22:27, Tom Lane  a écrit :
>> I'm checking it in HEAD though; perhaps there's something else wrong
>> in the back branches?

> That's also what I was thinking. I was only trying with v14. I just checked
> with v15devel, and your patch works alright. So there must be something
> else with back branches.

AFAICT the patch fixes what it intends to fix in v14 too.  The reason the
residual leak is worse in v14 is that the sinval message queue is bulkier.
We improved that in HEAD in commit 3aafc030a.  I'm not sure if I want to
take the risk of back-patching that, even now that it's aged a couple
months in the tree.  It is a pretty localized fix, but it makes some
assumptions about usage patterns that might not hold up.

regards, tom lane

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Greg Nancarrow]

On Tue, Nov 30, 2021 at 7:56 AM Tom Lane  wrote:
>
> After some further hackery, here's a set of patches that I think
> might be acceptable.  They're actually fairly independent, although
> they touch different aspects of the same behavior.
>
> 0001 gets rid of psqlscan.l's habit of suppressing dash-dash comments,
> but only once we have collected some non-whitespace query input.
> The upshot of this is that dash-dash comments will get sent to the
> server as long as they are within the query proper, that is after the
> first non-whitespace token and before the ending semicolon.  Comments
> that are between queries are still suppressed, because not doing that
> seems to result in far too much behavioral change.  As it stands,
> though, there are just a few regression test result changes.
>
> 0002 is a simplified version of Greg's patch.  I think we only need
> to look at the state of the query_buf to see if any input has been
> collected in order to determine if we are within or between queries.
> I'd originally thought this'd need to be a lot more complicated,
> but as long as psqlscan.l continues to drop pre-query comments,
> this seems to be enough.
>
> 0003 is the same patch I posted before to adjust M-# behavior.
>

I did some testing of the patches against the 4 problems that I
originally reported, and they fixed all of them.
0002 is definitely simpler than my original effort.
The patches LGTM.
Thanks for working on this.
(BTW, the patches are in Windows CRLF format, so on Linux at least I
needed to convert them using dos2unix so they'd apply using Git)

Regards,
Greg Nancarrow
Fujitsu Australia

Re: improve CREATE EXTENSION error message

[Chapman Flack]

On 11/29/21 17:54, Bossart, Nathan wrote:

> postgres=# CREATE EXTENSION does_not_exist;
> ERROR:  extension "does_not_exist" is not available
> DETAIL:  Extension control file 
> "/usr/local/pgsql/share/extension/does_not_exist.control" does not exist.
> HINT:  The extension must first be installed on the system where 
> PostgreSQL is running.

That looks like the direction I would have gone with it.

I wonder, though, is it better to write "does not exist." in the message,
or to use %m and get the exact message from the OS (which presumably would
be "No such file or directory" on Unix, and whatever Windows says for such
things on Windows).

My leaning is generally to use %m and therefore the exact OS message
in the detail, but I don't claim to speak for the project style on that.

Regards,
-Chap

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Alvaro Herrera]

On 2021-Nov-29, Tom Lane wrote:

> After some further hackery, here's a set of patches that I think
> might be acceptable.  They're actually fairly independent, although
> they touch different aspects of the same behavior.

I tried the collection and I find the overall behavior quite convenient.
I think it works just as I wish it would.

-- 
Álvaro Herrera   39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Tom Lane]

Greg Nancarrow  writes:
> (BTW, the patches are in Windows CRLF format, so on Linux at least I
> needed to convert them using dos2unix so they'd apply using Git)

Hmm.  Applying "od -c" to the copy of that message that's in my
PG list folder shows clearly that there's no \r in it, nor do
I see any when I save off the attachment.  I suppose this must
be an artifact of the way that your MUA treats text attachments;
or maybe the mail got mangled on its way to you.

regards, tom lane

Re: Can I assume relation would not be invalid during from ExecutorRun to ExecutorEnd

[Andy Fan]

On Mon, Nov 29, 2021 at 10:33 PM Tom Lane  wrote:

> Andy Fan  writes:
> > During my recent work,  I need some new stuff attached to Relation.
> Rather
> > than adding
> > some new data structures,  I added it to Relation directly.  Like
> > relation->balabala.  Then
> > I initialize it during ExecutorRun,  like  table_tuple_insert.. and
> > destroy it at ExecutorEnd.
>


> at least not if you expect that a relcache reset would not preserve the
> data.


Thanks for looking into this question.

I am not clear about this sentence .  IMO,  if the relcache is reset,  my
data
would be lost,  That's why I expect there is no relcache reset happening in
Executor Stage,  are you talking about something different?


> Also, what happens in the case of nested executor runs touching the same
> relation?
>

If you are talking about the RelationClose would be called many times, then
my solution is my resource is only destroyed when the refcount == 0;   If
you are talking
about the different situation needs different resource types, then that's
something
I didn't describe it clearly.  At the current time, we can assume "For 1
SQL statement, there
are only 1 resource needed per relation, even the executor or nest executor
touch the
same relation many times".   I'd like to describe this clearly as well for
review purposes,
but I want to focus on one topic only first.  So let's first assume my
assumption is correct.


> Why do you think this ought to be in the relcache, and not in the
> executor's rangetable-associated data structures?
>
>
I just see the ExecutorEnd code is not called after the exceptions case.
and I hope my resource
can be released totally even if the statement raises errors.  for example:

CREATE TABLE t(A INT PRIMARY KEY);

INSERT INTO t VALUES(1);
INSERT INTO t VALUES(1);

In the above case, the ExecutorEnd is not called, but the RelationClose
will be absolutely called
during the ResourceOwnerRelease call.

-- 
Best Regards
Andy Fan

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

[Cary Huang]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:not tested

Hi

The patch applies and tests fine. I don't think there is any harm having little 
extra statistical information about the checkpoint process. In fact, it could 
be useful in identifying a bottleneck during the checkpoint process as the 
stats exactly the time taken to do the file IO in pg_logical dir. 

best

Cary Huang

Re: Lots of memory allocated when reassigning Large Objects

[Justin Pryzby]

On Mon, Nov 29, 2021 at 01:40:31PM -0500, Tom Lane wrote:
> DROP OWNED BY likely has similar issues.

I tried a few more commands but found no significant issue.
IMO if you have 100k tables, then you can afford 1GB RAM.

SELECT format('CREATE TABLE t%s()', a) FROM generate_series(1,)a\gexec
SET client_min_messages=debug; SET log_statement_stats=on;

CREATE TABLESPACE tbsp LOCATION '/home/pryzbyj/tblspc';
ALTER TABLE ALL IN TABLESPACE pg_default SET TABLESPACE tbsp;
-- 10k tables uses 78MB RAM, which seems good enough (64MB the 2nd time??)

GRANT ALL ON ALL TABLES IN SCHEMA public TO current_user;
-- 10k tables uses 50MB RAM, which seems good enough
GRANT ALL ON ALL SEQUENCES IN SCHEMA public TO current_user
-- 10k sequences uses 47MB RAM, which seems good enough

SELECT format('CREATE FUNCTION f%s() RETURNS int RETURN 1;', a) FROM 
generate_series(1,)a;
GRANT ALL ON ALL FUNCTIONS IN SCHEMA public TO current_user;
-- 10k functions uses 62MB RAM, which seems good enough

And it looks like for ALTER PUBLICATION .. FOR ALL TABLES IN SCHEMA, the
namespace itself is stored, rather than enumerating all its tables.

>> IOW, it's asking for at least 481MB to reassign 1 million empty LO. It
>> strikes me as odd.

@Guillaume: Even if memory use with the patch isn't constant, I imagine it's
enough to have avoided OOM.

-- 
Justin

Re: Can I assume relation would not be invalid during from ExecutorRun to ExecutorEnd

[Andy Fan]

On Mon, Nov 29, 2021 at 10:56 PM Robert Haas  wrote:

> On Mon, Nov 29, 2021 at 2:10 AM Andy Fan  wrote:
> > 1.  During the ExecutorRun & ExecutorEnd,  the relcache will never by
> invalidated, if not
> > the old relation->balabala will be lost.  I assume this is correct since
> I didn't see any places
> > where we handle such changes in Executor code.
>
> It's not correct. We accept invalidation messages in a number of
> different code paths, including whenever we acquire a lock on a
> relation. That doesn't typically happen in the middle of a query, but
> that's just because most queries don't happen to do anything that
> would make it happen. They can, though.


Thanks for looking into this question.

Actually I think this would not happen.  My reasons are:

1. I think the case which would cause the relcache reset needs a lock, after
the executor start,  the executor lock is acquired so on one can really
have chances
to send an invalidation message until the lock is released. (let me first
ignore the 2
examples you talked about, and I will talk about it later).

2. _If_ the relation can be reset after we open it during Executor code,
then would the
relation (RelationData *) pointed memory still validated after the relcache
reset?  For example

CREATE TABLE t(a int);
INSERT INTO t VALUES(1), (2);

UPDATE t set a = 100;

We need to update 2 tuples in the update statement, if the relcache is
reset,  can we still use the previous
(RelationData *) to do the following update?  If not, what code is used to
change the relation for the old relcache
address to the new relcache.   I assumed (RelationData *) pointer. to the
relcache directly, hope this is correct..

For example, the query can
> call a user-defined function that accesses a table not previously
> touched by the transaction. Or a built-in function that does the same
> thing, like table_to_xml().
>
>
OK, this is something I missed before.  but looks it would not caused
different _in my case_;

IIUC, you are describing the thing like this:

CREATE FUNCTION udf()
...
SELECT * FROM t2;
$$;

SELECT udf() FROM t1;

then the relation t2 will not be opened at the beginning of ExecutorRun and
it will be only opened
when we fetch the first tuple from t1;  so we can have cache invalidation
between ExecutorRun and
the first call of udf.

But in my case, my exception should be that the relcache should not be
invalidated _after the first relation open_
in the executor (not the beginning of executorRun), this is something I
didn't describe well when I post
my message since I didn't find out this situation at that time.


-- 
Best Regards
Andy Fan

Re: Can I assume relation would not be invalid during from ExecutorRun to ExecutorEnd

[Andy Fan]

>
>
>
>> Why do you think this ought to be in the relcache, and not in the
>> executor's rangetable-associated data structures?
>>
>
I re-think about this,  I guess I didn't mention something  clear enough.
That's true that I bound my bala struct to Relation struct, and the memory
relation used  is allocated in relcache.  but the memory of bala is
allocated in
TopTransactionMemory context.

xxx_table_tuple_insert(Relation rel, ...)
{
   if (rel->balabala == NULL)
rel->balabala = allocate_bala_resource(rel);  //
*TopTransactionContext*.
   do_task_with(rel->balabala);
}

not sure if this should be called as putting my data in relcache.

and I rechecked the RelationData struct, and it looks like some
Executor-bind struct also
resides in it. for example: RelationData.rd_lookInfo.  If the relcache can
be reset, the
fields like this are unsafe to access as well.  Am I missing something?

-- 
Best Regards
Andy Fan

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Greg Nancarrow]

On Tue, Nov 30, 2021 at 11:08 AM Tom Lane  wrote:
>
> Greg Nancarrow  writes:
> > (BTW, the patches are in Windows CRLF format, so on Linux at least I
> > needed to convert them using dos2unix so they'd apply using Git)
>
> Hmm.  Applying "od -c" to the copy of that message that's in my
> PG list folder shows clearly that there's no \r in it, nor do
> I see any when I save off the attachment.  I suppose this must
> be an artifact of the way that your MUA treats text attachments;
> or maybe the mail got mangled on its way to you.
>

Yeah, sorry, looks like it could be a Gmail issue for me.
When I alternatively downloaded your patches from the pgsql-hackers
archive, they're in Unix format, as you say.
After a bit of investigation, it seems that patch attachments (like
yours) with a Context-Type of "text/x-diff" download through Gmail in
CRLF format for me (I'm running a browser on Windows, but my Postgres
development environment is in a Linux VM). So those must get converted
from Unix to CRLF format if downloaded using a browser running on
Windows.
The majority of patch attachments (?) seem to have a Context-Type of
"application/octet-stream" or "text/x-patch", and these seem to
download raw (in their original Unix format).
I guess the attachment context-type is varying according to the mail
client used for posting.

Sorry for the noise.

Regards,
Greg Nancarrow
Fujitsu Australia

RE: row filtering for logical replication

[houzj.f...@fujitsu.com]

On Mon, Nov 29, 2021 6:11 PM Amit Kapila  wrote:
> On Mon, Nov 29, 2021 at 12:10 PM Greg Nancarrow 
> wrote:
> >
> > On Fri, Nov 26, 2021 at 12:40 AM houzj.f...@fujitsu.com
> >  wrote:
> > >
> > > When researching and writing a top-up patch about this.
> > > I found a possible issue which I'd like to confirm first.
> > >
> > > It's possible the table is published in two publications A and B,
> > > publication A only publish "insert" , publication B publish
> > > "update". When UPDATE, both row filter in A and B will be executed. Is 
> > > this
> behavior expected?
> > >
> > > For example:
> > >  Publication
> > > create table tbl1 (a int primary key, b int); create publication A
> > > for table tbl1 where (b<2) with(publish='insert'); create
> > > publication B for table tbl1 where (a>1) with(publish='update');
> > >
> > >  Subscription
> > > create table tbl1 (a int primary key); CREATE SUBSCRIPTION sub
> > > CONNECTION 'dbname=postgres host=localhost port=1'
> PUBLICATION
> > > A,B;
> > >
> > >  Publication
> > > update tbl1 set a = 2;
> > >
> > > The publication can be created, and when UPDATE, the rowfilter in A
> > > (b<2) will also been executed but the column in it is not part of replica
> identity.
> > > (I am not against this behavior just confirm)
> > >
> >
> > There seems to be problems related to allowing the row filter to
> > include columns that are not part of the replica identity (in the case
> > of publish=insert).
> > In your example scenario, the tbl1 WHERE clause "(b < 2)" for
> > publication A, that publishes inserts only, causes a problem, because
> > column "b" is not part of the replica identity.
> > To see this, follow the simple example below:
> > (and note, for the Subscription, the provided tbl1 definition has an
> > error, it should also include the 2nd column "b int", same as in the
> > publisher)
> >
> >  Publisher:
> > INSERT INTO tbl1 VALUES (1,1);
> > UPDATE tbl1 SET a = 2;
> >
> > Prior to the UPDATE above:
> > On pub side, tbl1 contains (1,1).
> > On sub side, tbl1 contains (1,1)
> >
> > After the above UPDATE:
> > On pub side, tbl1 contains (2,1).
> > On sub side, tbl1 contains (1,1), (2,1)
> >
> > So the UPDATE on the pub side has resulted in an INSERT of (2,1) on
> > the sub side.
> >
> > This is because when (1,1) is UPDATEd to (2,1), it attempts to use the
> > "insert" filter "(b<2)" to determine whether the old value had been
> > inserted (published to subscriber), but finds there is no "b" value
> > (because it only uses RI cols for UPDATE) and so has to assume the old
> > tuple doesn't exist on the subscriber, hence the UPDATE ends up doing
> > an INSERT.
> > INow if the use of RI cols were enforced for the insert filter case,
> > we'd properly know the answer as to whether the old row value had been
> > published and it would have correctly performed an UPDATE instead of
> > an INSERT in this case.
> >
> 
> I don't think it is a good idea to combine the row-filter from the publication
> that publishes just 'insert' with the row-filter that publishes 'updates'. We
> shouldn't apply the 'insert' filter for 'update' and similarly for publication
> operations. We can combine the filters when the published operations are the
> same. So, this means that we might need to cache multiple row-filters but I
> think that is better than having another restriction that publish operation
> 'insert'
> should also honor RI columns restriction.

Personally, I agreed that an UPDATE operation should only apply a row filter 
that
is part of a publication that has only UPDATE.

Best regards,
Hou zj

Re: Fix typos

[Michael Paquier]

On Mon, Nov 29, 2021 at 01:01:55AM +, qianglj.f...@fujitsu.com wrote:
> I found several typos in comments and README. See patch attached.

Thanks, fixed.
--
Michael


signature.asc
Description: PGP signature

Re: Correct handling of blank/commented lines in PSQL interactive-mode history

[Greg Nancarrow]

On Tue, Nov 30, 2021 at 12:15 PM Greg Nancarrow  wrote:
>
> Yeah, sorry, looks like it could be a Gmail issue for me.
> When I alternatively downloaded your patches from the pgsql-hackers
> archive, they're in Unix format, as you say.
> After a bit of investigation, it seems that patch attachments (like
> yours) with a Context-Type of "text/x-diff" download through Gmail in
> CRLF format for me (I'm running a browser on Windows, but my Postgres
> development environment is in a Linux VM). So those must get converted
> from Unix to CRLF format if downloaded using a browser running on
> Windows.
> The majority of patch attachments (?) seem to have a Context-Type of
> "application/octet-stream" or "text/x-patch", and these seem to
> download raw (in their original Unix format).
> I guess the attachment context-type is varying according to the mail
> client used for posting.
>

Oops, typos, I meant to say "Content-Type".

Regards,
Greg Nancarrow
Fujitsu Australia

Re: Unifying VACUUM VERBOSE and log_autovacuum_min_duration output

[Peter Geoghegan]

On Fri, Nov 26, 2021 at 12:37 PM Peter Geoghegan  wrote:
> My preferred approach to this is simple: redefine VACUUM VERBOSE to
> not show incremental output, which seems rather unhelpful anyway. This
> does mean that VACUUM VERBOSE won't show certain information that
> might occasionally be useful to hackers.

Attached is a WIP patch doing this.

One thing that's still unclear is what the new elevel should be for
the ereport messages that used to be either LOG (for VACUUM VERBOSE)
or DEBUG2 (for everything else) -- what should I change them to now?
For now I've done taken the obvious approach of making everything
DEBUG2. There is of course no reason why some messages can't be DEBUG1
instead. Some of them do seem more interesting than others (though
still not particularly interesting overall).

Here is an example of VACUUM VERBOSE on HEAD:

pg@regression=# vacuum VERBOSE foo;
INFO:  vacuuming "public.foo"
INFO:  table "public.foo": found 0 removable, 54 nonremovable row
versions in 1 out of 45 pages
DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 770
Skipped 0 pages due to buffer pins, 0 frozen pages.
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
VACUUM

Here's what a VACUUM VERBOSE against the same table looks like with
the patch applied:

pg@regression=# vacuum VERBOSE foo;
INFO:  vacuuming "regression.public.foo"
INFO:  finished vacuuming "regression.public.foo": index scans: 0
pages: 0 removed, 45 remain, 0 skipped due to pins, 0 skipped frozen
tuples: 0 removed, 7042 remain, 0 are dead but not yet removable,
oldest xmin: 770
index scan not needed: 0 pages from table (0.00% of total) had 0 dead
item identifiers removed
I/O timings: read: 0.065 ms, write: 0.000 ms
avg read rate: 147.406 MB/s, avg write rate: 14.741 MB/s
buffer usage: 22 hits, 10 misses, 1 dirtied
WAL usage: 1 records, 1 full page images, 1401 bytes
system usage: CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
VACUUM

It's easy to produce examples where the patch is somewhat more verbose
than HEAD (that's what you see here). It's also easy to produce
examples where HEAD is *significantly* more verbose than the patch.
Especially when VERBOSE shows many lines of getrusage() output (patch
only ever shows one of those, at the end). Another factor is index
vacuuming. With the patch, you'll only see one extra line per index,
versus several lines on HEAD.

I cannot find clear guidelines on multiline INFO messages lines -- all
I'm really doing here is selectively making the LOG output from
log_autovacuum_min_duration into INFO output for VACUUM VERBOSE
(actually there are 2 INFO messages per heap relation processed). It
would be nice if there was a clear message style precedent that I
could point to for this.

--
Peter Geoghegan


v1-0001-Unify-VACUUM-VERBOSE-and-log_autovacuum_min_durat.patch
Description: Binary data

Re: Rename dead_tuples to dead_items in vacuumlazy.c

[Masahiko Sawada]

On Tue, Nov 30, 2021 at 3:00 AM Peter Geoghegan  wrote:
>
> On Wed, Nov 24, 2021 at 4:48 AM Masahiko Sawada  wrote:
> > The patch renames dead tuples to dead items at some places and  to
> > dead TIDs at some places.
>
> > I think it's more consistent if we change it to one side. I prefer "dead 
> > items".
>
> I just pushed a version of the patch that still uses both terms when
> talking about dead_items.

Thanks! I'll change my parallel vacuum refactoring patch accordingly.

Regarding the commit, I think that there still is one place in
lazyvacuum.c where we can change "dead tuples” to "dead items”:

/*
 * Allocate the space for dead tuples.  Note that this handles parallel
 * VACUUM initialization as part of allocating shared memory space used
 * for dead_items.
 */
dead_items_alloc(vacrel, params->nworkers);
dead_items = vacrel->dead_items;

Also, the commit doesn't change both PROGRESS_VACUUM_MAX_DEAD_TUPLES
and PROGRESS_VACUUM_NUM_DEAD_TUPLES. Did you leave them on purpose?

Regards,

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

RE: row filtering for logical replication

[tanghy.f...@fujitsu.com]

On Thursday, November 25, 2021 11:22 AM Peter Smith  
wrote:
> 
> Thanks for all the review comments so far! We are endeavouring to keep
> pace with them.
> 
> All feedback is being tracked and we will fix and/or reply to everything ASAP.
> 
> Meanwhile, PSA the latest set of v42* patches.
> 
> This version was mostly a patch restructuring exercise but it also
> addresses some minor review comments in passing.
> 

Thanks for your patch.
I have two comments on the document in 0001 patch.

1.
+   New row is used and it contains all columns. A NULL value
+   causes the expression to evaluate to false; avoid using columns without

I don't quite understand this sentence 'A NULL value causes the expression to 
evaluate to false'. 
The expression contains NULL value can also return true. Could you be more 
specific?

For example:

postgres=# select null or true;
 ?column?
--
 t
(1 row)


2.
+   at all then all other filters become redundant. If the subscriber is a
+   PostgreSQL version before 15 then any row 
filtering
+   is ignored.

If the subscriber is a PostgreSQL version before 15, it seems row filtering will
be ignored only when copying initial data, the later changes will not be 
ignored in row
filtering. Should we make it clear in document?

Regards,
Tang

Re: Optionally automatically disable logical replication subscriptions on error

[Greg Nancarrow]

On Sat, Nov 27, 2021 at 1:36 AM osumi.takami...@fujitsu.com
 wrote:
>
> This v7 uses v26 of skip xid patch [1]
>

This patch no longer applies on the latest source.
Also, the patch is missing an update to doc/src/sgml/catalogs.sgml,
for the new "subdisableonerr" column of pg_subscription.


Regards,
Greg Nancarrow
Fujitsu Australia