Re: Support "Right Semi Join" plan shapes

[Li Japin]

Hi, Richard

> On Apr 25, 2024, at 11:28, Richard Guo  wrote:
> 
> Here is another rebase with a commit message to help review.  I also
> tweaked some comments.

Thank you for updating the patch, here are some comments on the v5 patch.

+   /*
+* For now we do not support RIGHT_SEMI join in mergejoin or nestloop
+* join.
+*/
+   if (jointype == JOIN_RIGHT_SEMI)
+   return;
+

How about adding some reasons here? 

+ * this is a right-semi join, or this is a right/right-anti/full join and
+ * there are nonmergejoinable join clauses.  The executor's mergejoin

Maybe we can put the right-semi join together with the right/right-anti/full
join.  Is there any other significance by putting it separately?


Maybe the following comments also should be updated. Right?

diff --git a/src/backend/optimizer/prep/prepjointree.c 
b/src/backend/optimizer/prep/prepjointree.c
index 5482ab85a7..791cbc551e 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -455,8 +455,8 @@ pull_up_sublinks_jointree_recurse(PlannerInfo *root, Node 
*jtnode,
  * point of the available_rels machinations is to ensure that we only
  * pull up quals for which that's okay.
  *
- * We don't expect to see any pre-existing JOIN_SEMI, JOIN_ANTI, or
- * JOIN_RIGHT_ANTI jointypes here.
+ * We don't expect to see any pre-existing JOIN_SEMI, JOIN_ANTI,
+ * JOIN_RIGHT_SEMI, or JOIN_RIGHT_ANTI jointypes here.
  */
 switch (j->jointype)
 {
@@ -2951,6 +2951,7 @@ reduce_outer_joins_pass2(Node *jtnode,
  * so there's no way that upper quals could refer to their
  * righthand sides, and no point in checking.  We don't expect
  * to see JOIN_RIGHT_ANTI yet.
+ * Does JOIN_RIGHT_SEMI is expected here?
  */
 break;
 default:

Re: Thoughts about NUM_BUFFER_PARTITIONS

[Li Japin]

> On Feb 10, 2024, at 20:15, Tomas Vondra  wrote:
> 
> On 2/8/24 14:27, wenhui qiu wrote:
>> Hi Heikki Linnakangas
>>I think the larger shared buffer  higher the probability of multiple
>> backend processes accessing the same bucket slot BufMappingLock
>> simultaneously, （   InitBufTable(NBuffers + NUM_BUFFER_PARTITIONS); When I
>> have free time, I want to do this test. I have seen some tests, but the
>> result report is in Chinese
>> 
> 
> I think Heikki is right this is unrelated to the amount of RAM. The
> partitions are meant to reduce the number of lock collisions when
> multiple processes try to map a buffer concurrently. But the machines
> got much larger in this regard too - in 2006 the common CPUs had maybe
> 2-4 cores, now it's common to have CPUs with ~100 cores, and systems
> with multiple of them. OTOH the time spent holing the partition lock
> should be pretty low, IIRC we pretty much just pin the buffer before
> releasing it, and the backend should do plenty other expensive stuff. So
> who knows how many backends end up doing the locking at the same time.
> 
> OTOH, with 128 partitions it takes just 14 backends to have 50% chance
> of a conflict, so with enough cores ... But how many partitions would be
> enough? With 1024 partitions it still takes only 38 backends to get 50%
> chance of a collision. Better, but considering we now have hundreds of
> cores, not sure if sufficient.
> 
> (Obviously, we probably want much lower probability of a collision, I
> only used 50% to illustrate the changes).
> 

I find it seems need to change MAX_SIMUL_LWLOCKS if we enlarge the 
NUM_BUFFER_PARTITIONS,
I didn’t find any comments to describe the relation between MAX_SIMUL_LWLOCKS 
and
NUM_BUFFER_PARTITIONS, am I missing someghing?

Re: Transaction timeout

[Li Japin]

> 在 2023年12月23日，11:35，Junwang Zhao  写道：
> 
> On Sat, Dec 23, 2023 at 11:17 AM Japin Li  wrote:
>> 
>> a
>>> On Sat, 23 Dec 2023 at 10:40, Japin Li  wrote:
>>> On Sat, 23 Dec 2023 at 08:32, Japin Li  wrote:
 On Fri, 22 Dec 2023 at 23:30, Junwang Zhao  wrote:
> On Fri, Dec 22, 2023 at 10:44 PM Japin Li  wrote:
>> 
>> 
>> On Fri, 22 Dec 2023 at 22:37, Junwang Zhao  wrote:
>>> On Fri, Dec 22, 2023 at 10:25 PM Japin Li  wrote:
 I try to set idle_in_transaction_session_timeout after begin 
 transaction,
 it changes immediately, so I think transaction_timeout should also be 
 take
 immediately.
>>> 
>>> Ah, right, idle_in_transaction_session_timeout is set after the set
>>> command finishes and before the backend send *ready for query*
>>> to the client, so the value of the GUC is already set before
>>> next command.
>>> 
>> 
>> I mean, is it possible to set transaction_timeout before next comand?
>> 
> Yeah, it's possible, set transaction_timeout in the when it first
> goes into *idle in transaction* mode, see the attached files.
> 
 
 Thanks for updating the patch, LGTM.
>>> 
>>> Sorry for the noise!
>>> 
>>> Read the previous threads, I find why the author enable transaction_timeout
>>> in start_xact_command().
>>> 
>>> The v15 patch cannot handle COMMIT AND CHAIN, see [1]. For example:
>>> 
>>> SET transaction_timeout TO '2s'; BEGIN; SELECT 1, pg_sleep(1); COMMIT AND 
>>> CHAIN; SELECT 2, pg_sleep(1); COMMIT;
>>> 
>>> The transaction_timeout do not reset when executing COMMIT AND CHAIN.
>>> 
>>> [1] 
>>> https://www.postgresql.org/message-id/a906dea1-76a1-4f26-76c5-a7efad3ef5b8%40oss.nttdata.com
>> 
>> Attach v16 to solve this. Any suggestions?
> 
> I've checked this with *COMMIT AND CHAIN* and *ABORT AND CHAIN*,
> both work as expected. Thanks for the update.
> 

Thanks for your testing and reviewing!

Re: redundant check of msg in does_not_exist_skipping

[Li Japin]

> On Nov 18, 2022, at 4:04 AM, Robert Haas  wrote:
> 
> On Thu, Nov 17, 2022 at 10:56 AM Tom Lane  wrote:
>> This is a completely bad idea.  If it takes that level of analysis
>> to see that msg can't be null, we should leave the test in place.
>> Any future modification of either this code or what it calls could
>> break the conclusion.
> 
> +1. Also, even if the check were quite obviously useless, it's cheap
> insurance. It's difficult to believe that it hurts performance in any
> measurable way. If anything, we would benefit from having more sanity
> checks in our code, rather than fewer.
> 

Thanks for the explanation! Got it.

Re: Built-in connection pooler

[Li Japin]

On Mar 22, 2021, at 4:59 AM, Zhihong Yu 
mailto:z...@yugabyte.com>> wrote:

Hi,

+  With load-balancing policy postmaster choose 
proxy with lowest load average.
+  Load average of proxy is estimated by number of clients connection 
assigned to this proxy with extra weight for SSL connections.

I think 'load-balanced' may be better than 'load-balancing'.
postmaster choose proxy -> postmaster chooses proxy

+  Load average of proxy is estimated by number of clients connection 
assigned to this proxy with extra weight for SSL connections.

I wonder if there would be a mixture of connections with and without SSL.

+ Terminate an idle connection pool worker after the specified number 
of milliseconds.

Should the time unit be seconds ? It seems a worker would exist for at least a 
second.

+ * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group

It would be better to update the year in the header.

+* Use then for launching pooler worker backends and report error

Not sure I understand the above sentence. Did you mean 'them' instead of 'then' 
?

Cheers

On Sun, Mar 21, 2021 at 11:32 AM Konstantin Knizhnik 
mailto:knizh...@garret.ru>> wrote:
People asked me to resubmit built-in connection pooler patch to commitfest.
Rebased version of connection pooler is attached.


Hi, hackers

Does the PostgreSQL core do not interested in the built-in connection pool? If 
so, could
somebody tell me why we do not need it? If not, how can we do for this to make 
it in core?

Thanks in advance!

[Sorry if you already receive this email, since I typo an invalid pgsql list 
email address in previous email.]

--
Best regards
Japin Li

Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION

[Li Japin]

> On Aug 7, 2021, at 1:35 PM, Amit Kapila  wrote:
> 
> On Fri, Aug 6, 2021 at 9:57 PM Japin Li  wrote:
>> 
>>> 
>>> Hmm yes, it cannot cover all cases. I had somehow misunderstood that
>>> the subscriber knows which relations are associated with which
>>> publications. Given that the subscriber doesn’t know which relations
>>> are associated with which publications (and the set of subscribed
>>> relations on the subscriber becomes out of date once the publication
>>> is updated), I think it's impossible to achieve that DROP PUBLICATION
>>> drops relations that are associated with only the publication being
>>> dropped.
>>> 
 Do we have better ideas to fix or shall we just
 say that we will refresh based on whatever current set of relations
 are present in publication to be dropped?
>>> 
>>> I don't have better ideas. It seems to me that it's prudent that we
>>> accept the approach in v3 patch and refresh all publications in DROP
>>> PUBLICATION cases.
>>> 
>> 
>> Maybe refreshing all publications in PUBLICATION cases is simple way to
>> fix the problem.
>> 
> 
> Do you mean to say that do it for both Add and Drop or just for Drop?
> Actually, doing it both will make the behavior consistent but doing it
> just for Drop might be preferable by some users. I think it is better
> to be consistent here but I am fine if you and others feel it is
> better to refresh all publications only in Drop case.
> 

I prefer to refresh all PUBLICATIONS for both ADD and DROP operations, I think
this is more consistent and makes the code simple.

--
Best regards
Japin Li

Re: WIP: System Versioned Temporal Table

[Li Japin]

On Jan 27, 2021, at 12:39 AM, Surafel Temesgen 
mailto:surafel3...@gmail.com>> wrote:



On Tue, Jan 26, 2021 at 2:33 PM Vik Fearing 
mailto:v...@postgresfriends.org>> wrote:
I'm still in the weeds of reviewing this patch, but why should this
fail?  It should not fail.

Attached is rebased patch that include isolation test


Thanks for updating the patch.  However it cannot apply to master (e5d8a9990).

Here are some comments on system-versioning-temporal-table_2021_v13.patch.

+
+When system versioning is specified two columns are added which
+record the start timestamp and end timestamp of each row verson.

verson -> version

+By default, the column names will be StartTime and EndTime, though
+you can specify different names if you choose.

In fact, it is start_time and end_time, not StartTime and EndTime.
I think it's better to use  label around start_time and end_time.

+column will be automatically added to the Primary Key of the
+table.

Should we mention the unique constraints?

+The system versioning period end column will be added to the
+Primary Key of the table as a way of ensuring that concurrent
+INSERTs conflict correctly.

Same as above.

Since the get_row_start_time_col_name() and get_row_end_time_col_name()
are similar, IMO we can pass a flag to get StartTime/EndTime column name,
thought?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

[Li Japin]

> On Jan 27, 2021, at 19:41, Bharath Rupireddy 
>  wrote:
> 
> On Wed, Jan 27, 2021 at 4:42 PM Amit Kapila  wrote:
>>> On Wed, Jan 27, 2021 at 3:16 PM Bharath Rupireddy
>>>  wrote:
 On Wed, Jan 27, 2021 at 3:01 PM Amit Kapila  
 wrote:
>>> So, I think the new syntax, ALTER SUBSCRIPTION .. ADD/DROP PUBLICATION
>>> will refresh the new and existing publications.
>> That sounds a bit unusual to me because when the user has specifically
>> asked to just ADD Publication, we might refresh some existing
>> Publication along with it?
> 
> Hmm. That's correct. I also feel we should not touch the existing
> publications, only the ones that are added/dropped should be
> refreshed. Because there will be an overhead of a SQL with more
> publications(in fetch_table_list) when AlterSubscription_refresh() is
> called with all the existing publications. We could just pass in the
> newly added/dropped publications to AlterSubscription_refresh().
> 
> I don't see any problem if ALTER SUBSCRIPTION ... ADD PUBLICATION with
> refresh true refreshes only the newly added publications, because what
> we do in AlterSubscription_refresh() is that we fetch the tables
> associated with the publications from the publisher, compare them with
> the previously fetched tables from that publication and add the new
> tables or remove the table that don't exist in that publication
> anymore.
> 
> For ALTER SUBSCRIPTION ... DROP PUBLICATION, also we can do the same
> thing i.e. refreshes only the dropped publications.
> 
> Thoughts?

Argeed. We just only need to refresh the added/dropped publications.  
Furthermore, for dropped publications we do not need the “copy_data” option, 
right?

> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com

Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

[Li Japin]

> On Jan 27, 2021, at 19:41, Bharath Rupireddy 
>  wrote:
> 
> On Wed, Jan 27, 2021 at 4:42 PM Amit Kapila  wrote:
>>> On Wed, Jan 27, 2021 at 3:16 PM Bharath Rupireddy
>>>  wrote:
 On Wed, Jan 27, 2021 at 3:01 PM Amit Kapila  
 wrote:
>>> So, I think the new syntax, ALTER SUBSCRIPTION .. ADD/DROP PUBLICATION
>>> will refresh the new and existing publications.
>> That sounds a bit unusual to me because when the user has specifically
>> asked to just ADD Publication, we might refresh some existing
>> Publication along with it?
> 
> Hmm. That's correct. I also feel we should not touch the existing
> publications, only the ones that are added/dropped should be
> refreshed. Because there will be an overhead of a SQL with more
> publications(in fetch_table_list) when AlterSubscription_refresh() is
> called with all the existing publications. We could just pass in the
> newly added/dropped publications to AlterSubscription_refresh().
> 
> I don't see any problem if ALTER SUBSCRIPTION ... ADD PUBLICATION with
> refresh true refreshes only the newly added publications, because what
> we do in AlterSubscription_refresh() is that we fetch the tables
> associated with the publications from the publisher, compare them with
> the previously fetched tables from that publication and add the new
> tables or remove the table that don't exist in that publication
> anymore.
> 
> For ALTER SUBSCRIPTION ... DROP PUBLICATION, also we can do the same
> thing i.e. refreshes only the dropped publications.
> 
> Thoughts?

Agreed. We just only need to refresh the added/dropped publications.  
Furthermore, for publications that will be dropped, we do not need the 
“copy_data” option, right?

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Logical Replication - behavior of ALTER PUBLICATION .. DROP TABLE and ALTER SUBSCRIPTION .. REFRESH PUBLICATION

[Li Japin]

> On Jan 14, 2021, at 8:44 PM, Amit Kapila  wrote:
> 
> On Thu, Jan 14, 2021 at 6:02 PM japin  wrote:
>> 
>> On Thu, 14 Jan 2021 at 20:19, Bharath Rupireddy wrote:
>>> On Thu, Jan 14, 2021 at 5:36 PM Li Japin  wrote
>>>> Do we really need to access PUBLICATIONRELMAP in this patch? What if
>>>> we just set it to false in the else condition of (if (publish &&
>>>> (relkind != RELKIND_PARTITIONED_TABLE || pub->pubviaroot)))
>>>> 
>>>> Thank for you review. I agree with you, it doesn’t need to access 
>>>> PUBLICATIONRELMAP, since
>>>> We already get the publication oid in GetRelationPublications(relid) [1], 
>>>> which also access
>>>> PUBLICATIONRELMAP.  If the current pub->oid does not in pubids, the 
>>>> publish is false, so we
>>>> do not need publish the table.
>>> 
>>> +1. This is enough.
>>> 
>>>> I have another question, the data->publications is a list, when did it has 
>>>> more then one items?
>>> 
>>> IIUC, when the single table is associated with multiple publications,
>>> then data->publications will have multiple entries. Though I have not
>>> tried, we can try having two or three publications for the same table
>>> and verify that.
>>> 
>> 
>> I try add one table into two publications, but the data->publications has 
>> only
>> one item.  Is there something I missed?
>> 
> 
> I think you will have multiple publications in that list when the
> subscriber has subscribed to multiple publications. For example,
> Create Subscription ... Publication pub1, Publication pub2.
> 

Thanks, you are right! When we create a subscription with multiple publications,
the data->publications has more then one items.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Logical Replication - behavior of ALTER PUBLICATION .. DROP TABLE and ALTER SUBSCRIPTION .. REFRESH PUBLICATION

[Li Japin]

On Jan 14, 2021, at 1:25 PM, Amit Kapila 
mailto:amit.kapil...@gmail.com>> wrote:

On Wed, Jan 13, 2021 at 5:40 PM Bharath Rupireddy
mailto:bharath.rupireddyforpostg...@gmail.com>>
 wrote:

On Wed, Jan 13, 2021 at 3:16 PM Bharath Rupireddy
mailto:bharath.rupireddyforpostg...@gmail.com>>
 wrote:

On Wed, Jan 13, 2021 at 2:36 PM japin 
mailto:japi...@hotmail.com>> wrote:
In summary, I feel we need to fix the publisher sending the inserts
even though the table is dropped from the publication, that is the
patch patch proposed by japin. This solves the bug reported in this
thread.

And also, it's good to have the LogicalRepRelMap invalidation fix as a
0002 patch in invalidate_syncing_table_states, subscription_change_cb
and logicalrep_rel_open as proposed by me.

Thoughts?


I think invalidate the LogicalRepRelMap is necessary.  If the table isn't in
subscription, can we remove the LogicalRepRelMapEntry from LogicalRepRelMap?

IIUC, it's not possible to know the relid of the alter
publication...dropped table in the invalidation callbacks
invalidate_syncing_table_states or subscription_change_cb, so it's
better to set state of all the entries to SUBREL_STATE_UNKNOWN, so
that, in the next logicalrep_rel_open call the function
GetSubscriptionRelState will be called and the pg_subscription_rel
will be read freshly.

This is inline with the reasoning specified at [1].

[1] - 
https://www.postgresql.org/message-id/CAFiTN-udwuc_h0TaFrSEKb-Yo1vVvkjz9TDRw7VE3P-KiPSGbQ%40mail.gmail.com

Attaching following two patches:

0001 - Makes publisher to not publish the changes for the alter
publication ... dropped tables. Original patch is by japin, I added
comments, changed the function name and adjusted the code a bit.


Do we really need to access PUBLICATIONRELMAP in this patch? What if
we just set it to false in the else condition of (if (publish &&
(relkind != RELKIND_PARTITIONED_TABLE || pub->pubviaroot)))


Thank for you review. I agree with you, it doesn’t need to access 
PUBLICATIONRELMAP, since
We already get the publication oid in GetRelationPublications(relid) [1], which 
also access
PUBLICATIONRELMAP.  If the current pub->oid does not in pubids, the publish is 
false, so we
do not need publish the table.

I have another question, the data->publications is a list, when did it has more 
then one items?

0001 - change as you suggest.
0002 - does not change.

Please consider v2 for further review.

[1]
List *
GetRelationPublications(Oid relid)
{
List   *result = NIL;
CatCList   *pubrellist;
int i;

/* Find all publications associated with the relation. */
pubrellist = SearchSysCacheList1(PUBLICATIONRELMAP,
 ObjectIdGetDatum(relid));
for (i = 0; i < pubrellist->n_members; i++)
{
HeapTuple   tup = >members[i]->tuple;
Oid pubid = ((Form_pg_publication_rel) GETSTRUCT(tup))->prpubid;

result = lappend_oid(result, pubid);
}

ReleaseSysCacheList(pubrellist);

return result;
}


--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.



v2-0001-Fix-ALTER-PUBLICATION.DROP-TABLE-behaviour.patch
Description: v2-0001-Fix-ALTER-PUBLICATION.DROP-TABLE-behaviour.patch


v2-0002-Invalidate-relation-map-cache-in-subscriber-sysca.patch
Description:  v2-0002-Invalidate-relation-map-cache-in-subscriber-sysca.patch

Re: Fix typo about WalSndPrepareWrite

[Li Japin]

On Jan 14, 2021, at 12:56 PM, Ashutosh Bapat 
mailto:ashutosh.bapat@gmail.com>> wrote:

Hi Japin,
Thanks for the report.

I think that comment is correct. It refers to the following code
blocks of XLogSendPhysical()

2744 /*
2745  * OK to read and send the slice.
2746  */
2747 resetStringInfo(_message);
2748 pq_sendbyte(_message, 'w');
2749
2750 pq_sendint64(_message, startptr);/* dataStart */
2751 pq_sendint64(_message, SendRqstPtr); /* walEnd */
2752 pq_sendint64(_message, 0);   /* sendtime, filled in last */

2803  * Fill the send timestamp last, so that it is taken as late
as possible.
2804  */
2805 resetStringInfo();
2806 pq_sendint64(, GetCurrentTimestamp());
2807 memcpy(_message.data[1 + sizeof(int64) + sizeof(int64)],
2808tmpbuf.data, sizeof(int64));
2809
2810 pq_putmessage_noblock('d', output_message.data, output_message.len);


After a quick search, I found that WalSndPrepareWrite and WalSndWriteData are 
always pairs [1].
IIUC the space of sendtime leave by WalSndPrepareWrite, it always fill out by 
WalSndWriteData.


WalSndWriteData() also fills the timestamp there but it may not always
be used with WalSndPrepareWrite, at least theoretically. So it's the
XLogSendPhysical() that it's referring to.

Maybe a better way is to separate those two codeblocks into functions
and use it in WalSndPrepareWrite, WalSndWriteData and XLogSendPhysical
appropriately. That way we don't have to add a comment like that and
the sanity of 'w' message is preserved.


Sure, we can write a separate function to fill out the sendtime.  Any thoughts?


[1]
src/backend/replication/walsender.c:247:static void 
WalSndPrepareWrite(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId 
xid, bool last_write);
src/backend/replication/walsender.c:1015:   
WalSndPrepareWrite, WalSndWriteData,
src/backend/replication/walsender.c:1176:   
  WalSndPrepareWrite, WalSndWriteData,
src/backend/replication/walsender.c:1233:WalSndPrepareWrite(LogicalDecodingContext
 *ctx, XLogRecPtr lsn, TransactionId xid, bool last_write)
src/backend/replication/walsender.c:1255: * Actually write out data previously 
prepared by WalSndPrepareWrite out to

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Logical Replication - behavior of ALTER PUBLICATION .. DROP TABLE and ALTER SUBSCRIPTION .. REFRESH PUBLICATION

[Li Japin]

On Jan 12, 2021, at 5:47 PM, japin 
mailto:japi...@hotmail.com>> wrote:


On Tue, 12 Jan 2021 at 14:38, Amit Kapila wrote:
On Tue, Jan 12, 2021 at 11:39 AM Bharath Rupireddy
mailto:bharath.rupireddyforpostg...@gmail.com>>
 wrote:

On Tue, Jan 12, 2021 at 9:05 AM Amit Kapila 
mailto:amit.kapil...@gmail.com>> wrote:

On Mon, Jan 11, 2021 at 6:51 PM Bharath Rupireddy
mailto:bharath.rupireddyforpostg...@gmail.com>>
 wrote:

Hi,

While providing thoughts on the design in [1], I found a strange
behaviour with the $subject. The use case is shown below as a sequence
of steps that need to be run on publisher and subscriber to arrive at
the strange behaviour.  In step 5, the table is dropped from the
publication and in step 6, the refresh publication is run on the
subscriber, from here onwards, the expectation is that no further
inserts into the publisher table have to be replicated on to the
subscriber, but the opposite happens i.e. the inserts are still
replicated to the subscriber. ISTM as a bug. Let me know if I'm
missing anything.


Did you try to investigate what's going on? Can you please check what
is the behavior if, after step-5, you restart the subscriber and
separately try creating a new subscription (maybe on a different
server) for that publication after step-5 and see if that allows the
relation to be replicated? AFAIU, in AlterSubscription_refresh, we
remove such dropped rels and stop their corresponding apply workers
which should stop the further replication of such relations but that
doesn't seem to be happening in your case.

Here's my analysis:
1) in the publisher, alter publication drop table successfully
removes(PublicationDropTables) the table from the catalogue
pg_publication_rel
2) in the subscriber, alter subscription refresh publication
successfully removes the table from the catalogue pg_subscription_rel
(AlterSubscription_refresh->RemoveSubscriptionRel)
so far so good


Here, it should register the worker to stop on commit, and then on
commit it should call AtEOXact_ApplyLauncher to stop the apply worker.
Once the apply worker is stopped, the corresponding WALSender will
also be stopped. Something here is not happening as per expected
behavior.

3) after the insertion into the table in the publisher(remember that
it's dropped from the publication in (1)), the walsender process is
unable detect that the table has been dropped from the publication
i.e. it doesn't look at the pg_publication_rel catalogue or some
other, but it only does is_publishable_relation() check which returns
true in pgoutput_change(). Maybe the walsender should look at the
catalogue pg_publication_rel in is_publishable_relation()?


We must be somewhere checking pg_publication_rel before sending the
decoded change because otherwise, we would have sent the changes for
the table which are not even part of this publication. I think you can
try to create a separate table that is not part of the publication
under test and see how the changes for that are filtered.

I find that pgoutput_change() use a hash table RelationSyncCache to
cache the publication info for tables.  When we drop tables from the
publication, the RelationSyncCache doesn't updated, so it replicate
records.


IIUC the logical replication only replicate the tables in publication, I think
when the tables that aren't in publication should not be replicated.

Attached the patch that fixes it.  Thought?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.






alter-publication-drop-table.patch
Description: alter-publication-drop-table.patch

Re: Added schema level support for publication.

[Li Japin]

On Jan 11, 2021, at 5:06 PM, Bharath Rupireddy 
mailto:bharath.rupireddyforpostg...@gmail.com>>
 wrote:

On Mon, Jan 11, 2021 at 1:29 PM japin 
mailto:japi...@hotmail.com>> wrote:
Say a user has created a publication for a schema with hundreds of
tables in it, at some point later, can he stop replicating a single or
some tables from that schema?


There is no provision for this currently.

The documentation [1] says, we can ALTER PUBLICATION testpub DROP
TABLE t1; which removes the table from the list of published tables,
but looks like it requires ALTER SUBSCRIPTION testsub REFRESH
PUBLICATION; for the changes to become effective on the subscriber. I
have done some testing for this case:
1) created publication for table t1, see \d+ t1, the associated
publication is visible in the output
2) created subscription on the subscriber, initial available data from
the publisher for table t1 is received
3) insert into table t1 on the publisher
4) inserted data in (3) is received in the subscriber table t1
5) alter publication to drop the table t1 on the publisher, see \d+
t1, there will not be any associated publication in the output
6) execute alter subscription refresh publication on the subscriber,
with the expectation that it should not receive the data from the
publisher for the table t1 since it's dropped from the publication in
(5)
7) insert into table t1 on the publisher
8) still the newly inserted data in (7) from the publisher, will be
received into the table t1 in the subscriber

IIUC, the behaviour of ALTER PUBLICATION DROP TABLE from the docs and
the above use case, it looks like a bug to me. If I'm wrong, can
someone correct me?


Yes, if we modify the publication, we should refresh the subscription on
each subscriber.  It looks strange for me, especially for partitioned
tables [1].

Thoughts?


Can we trace the different between publication and subscription, and
auto-refresh subscription on subscriber?

[1]
https://www.postgresql.org/message-id/flat/1D6DCFD2-0F44-4A18-BF67-17C2697B1631%40hotmail.com

As Amit stated in your thread [1], DDLs like creation of the new
tables or partitions, schema changes etc. on the publisher can not be
replicated automatically by the logical replication framework to the
subscriber. Users have to perform those DDLs on the subscribers by
themselves.

Yeah, DDLs is not supported now. On publisher, the partitions are added to the
publication automatically.  However, even if we created the partitions on 
subscriber,
it will not sync the new partitions, because it likes normal table, we must 
execute
ALTER SUBSCRIPTION my_test REFRESH PUBLICATION;
I preferred it will automatically add to subscription when we create the new 
partitions
if the partitions is already in publication.

If your point is to at least issue the ALTER SUBSCRIPTION testsub
REFRESH PUBLICATION; from the publication whenever the publication is
altered i.e. added or dropped tables, IMO, we cannot do this, because
running this command on the subscriber only makes sense, after user
runs the same DDLs (which were run on the publisher) also on the
subscriber. To illustrate this:
1) create a new table or partition on the publisher and add it to
publisher, note that the same table has not yet been created on the
subscriber
2) imagine the publisher issuing an auto refresh command to all the
subscribers, then, no point in that right, because the new table or
the partition is not yet created on all the subscribers.

So, IMO, we can not have an auto refresh mechanism, until we have the
feature to replicate the DDL changes to all the subscribers.

Thanks for clarification.

What I stated in my earlier mail [1] is that even though we drop the
table from the publication in the publisher and run a refresh
publication on the subscriber, still the data is being replicated from
the publisher to the subscriber table. I just wanted to know whether
this is the expected behaviour or what exactly means. a user running
ALTER PUBLICATION mypub DROP TABLE mytable;

[1] - 
https://www.postgresql.org/message-id/CALj2ACWAxO3vSToT0o5nXL%3Drz5cNx90zaV-at%3DcvM14Tag4%3DcQ%40mail.gmail.com

Sorry, I misunderstood. After the test (ce6a71fa530). I found that if we do not 
insert data
between step (5) and (6), it will not ship the new records, however, if we 
insert
data between step (5) and (6), it will ship the new records.

(1) created publication for table t1, t2
postgres[8765]=# CREATE TABLE t1 (a int);
CREATE TABLE
postgres[8765]=# CREATE TABLE t2 (a int);
CREATE TABLE
postgres[8765]=# INSERT INTO t1 VALUES (1);
INSERT 0 1
postgres[8765]=# INSERT INTO t2 VALUES (1);
INSERT 0 1
postgres[8765]=# CREATE PUBLICATION mypub1 FOR TABLE t1;
CREATE PUBLICATION
postgres[8765]=# CREATE PUBLICATION mypub2 FOR TABLE t2;
CREATE PUBLICATION

(2) created subscription on the subscriber
postgres[9812]=# CREATE TABLE t1 (a int);
CREATE TABLE
postgres[9812]=# CREATE TABLE t2 (a int);
CREATE TABLE
postgres[9812]=# CREATE SUBSCRIPTION mysub1 CONNECTION 

Re: Terminate the idle sessions

[Li Japin]

--
Best regards
Japin Li

On Jan 7, 2021, at 10:03 AM, Thomas Munro 
mailto:thomas.mu...@gmail.com>> wrote:


* I'm not entirely comfortable with the name "idle_session_timeout",
because it sounds like it applies to all idle states, but actually
it only applies when we're not in a transaction.  I left the name
alone and tried to clarify the docs, but I wonder whether a different
name would be better.  (Alternatively, we could say that it does
apply in all cases, making the effective timeout when in a transaction
the smaller of the two GUCs.  But that'd be more complex to implement
and less flexible, so I'm not in favor of that answer.)

Hmm, it is a bit confusing, but having them separate is indeed more flexible.


Yes! It is a bit confusing. How about interactive_timeout? This is used by 
MySQL [1].

* The SQLSTATE you chose for the new error condition seems pretty
random.  I do not see it in the SQL standard, so using a code that's
within the spec-reserved code range is certainly wrong.  I went with
08P02 (the "P" makes it outside the reserved range), but I'm not
really happy either with using class 08 ("Connection Exception",
which seems to be mainly meant for connection-request failures),
or the fact that ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT is
practically identical but it's not even in the same major class.
Now 25 ("Invalid Transaction State") is certainly not right for
this new error, but I think what that's telling us is that 25 was a
misguided choice for the other error.  In a green field I think I'd
put both of them in class 53 ("Insufficient Resources") or maybe class
57 ("Operator Intervention").  Can we get away with renumbering the
older error at this point?  In any case I'd be inclined to move the
new error to 53 or 57 --- anybody have a preference which?

I don't have a strong view here, but 08 with a P doesn't seem crazy to
me.  Unlike eg 57014 (statement_timeout), 57017 (deadlock_timeout),
55P03 (lock_timeout... huh, I just noticed that DB2 uses 57017 for
that, distinguished from deadlock by another error field), after these
timeouts you don't have a session/connection anymore.  The two are a
bit different though: in the older one, you were in a transaction, and
it seems to me quite newsworthy that your transaction has been
aborted, information that is not conveyed quite so clearly with a
connection-related error class.

Apologize! I just think it is a Connection Exception.

[1] 
https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_interactive_timeout

Cannot ship records to subscriber for partition tables using logical replication (publish_via_partition_root=false)

[Li Japin]

Hi, hackers

When I use logical stream replication on partition table, I find that if we 
create a new
partitions after the subscription on subscriber,  the records in new partitions 
cannot be
shipped to the subscriber.

Here is an example:

1. Create a view to check the subscription tables.

```
— on subscriber
CREATE VIEW pg_subscription_tables AS
SELECT
s.subname,
n.nspname AS schemaname,
c.relname AS tablename
FROM
pg_subscription s JOIN pg_subscription_rel p ON s.oid = p.srsubid,
pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE c.oid = p.srrelid;
```

1. Create a publication and subscription.

```
— on publisher
CREATE TABLE test_parent (a int, b int) PARTITION BY RANGE (a);
CREATE TABLE test_child_01 PARTITION OF test_parent FOR VALUES FROM (1) TO (10);
CREATE PUBLICATION my_test_pub FOR TABLE test_parent;

— on subscriber
CREATE TABLE test_parent (a int, b int) PARTITION BY RANGE (a);
CREATE TABLE test_child_01 PARTITION OF test_parent FOR VALUES FROM (1) TO (10);
CREATE SUBSCRIPTION my_test_sub CONNECTION 'host=localhost port=8765 
dbname=postgres' PUBLICATION my_test_pub;
```

2. The insert data into test_parent on publisher, and everything looks good.

```
— on publisher
INSERT INTO test_parent VALUES (5, 50);
SELECT * FROM pg_publication_tables;
   pubname   | schemaname |   tablename
-++---
 my_test_pub | public | test_child_01
(1 row)

— on subscriber
SELECT * FROM test_parent;
 a | b
---+
 5 | 50
(1 row)

SELECT * FROM pg_subscription_tables;
   subname   | schemaname |   tablename
-++---
 my_test_sub | public | test_child_01
(1 row)
```

3. However, If we create a new partitions on both publisher and subscriber. And 
the records
in new partitions cannot ship to the subscriber. When I check the 
`pg_publication_tables`, I
found that the new partitions are already in publication. But on the 
subscriber, the
`pg_subscription_rel` do not have the new partitions.

```
— on publisher
CREATE TABLE test_child_02 PARTITION OF test_parent FOR VALUES FROM (10)  TO 
(20);
SELECT * FROM pg_publication_tables;
   pubname   | schemaname |   tablename
-++---
 my_test_pub | public | test_child_01
 my_test_pub | public | test_child_02
(2 rows)
INSERT INTO test_parent VALUES (15, 150);

— on subscriber
CREATE TABLE test_child_02 PARTITION OF test_parent FOR VALUES FROM (10)  TO 
(20);
SELECT * FROM test_parent;
 a | b
---+
 5 | 50
(1 row)

SELECT * FROM pg_subscription_tables;
   subname   | schemaname |   tablename
-++---
 my_test_sub | public | test_child_01
(1 row)
```

I think it looks strange. But if we create publication with 
`publish_via_partition_root` it work fine,
since all records are ship on the partitioned table [1].

When `publish_via_partition_root` is false, since the publisher add the new 
partitions in
publication,  should we add them on the subscriber automatically?


[1] https://www.postgresql.org/docs/devel/sql-createpublication.html

--
Best regards
Japin Li
ChengDu WenWu Information Technology Co.Ltd.

Re: Confused about stream replication protocol documentation

[Li Japin]

On Dec 23, 2020, at 8:11 PM, Fujii Masao 
mailto:masao.fu...@oss.nttdata.com>> wrote:


On 2020/12/23 11:08, Li Japin wrote:
On Dec 22, 2020, at 11:13 PM, Fujii Masao 
mailto:masao.fu...@oss.nttdata.com> 
<mailto:masao.fu...@oss.nttdata.com>> wrote:

‘B’ means a backend and ‘F’ means a frontend. Maybe as [1] does, we should
add the note like "Each is marked to indicate that it can be sent by
a frontend (F) and a backend (B)" into the description about each message
format for START_REPLICATION.

[1]
https://www.postgresql.org/docs/devel/protocol-message-formats.html 
<https://www.postgresql.org/docs/devel/protocol-message-formats.html>
Thanks for your clarify.  Maybe we should move the "protocol message formats”
before “stream replication protocol” or referenced it in "stream replication 
protocol”.

I like the latter. And maybe it's better to reference to also
"53.6. Message Data Types" there because the messages for
START_REPLICATION use the message data types.

Add reference about “protocol message types” and “protocol message formats”.

index 4899bacda7..5793936b42 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2069,8 +2069,9 @@ The commands accepted in replication mode are:
  

  
-  WAL data is sent as a series of CopyData messages.  (This allows
-  other information to be intermixed; in particular the server can send
+  WAL data is sent as a series of CopyData messages
+  (See  and ).
+  (This allows other information to be intermixed; in particular the 
server can send
   an ErrorResponse message if it encounters a failure after beginning
   to stream.)  The payload of each CopyData message from server to the
   client contains a message of one of the following formats:

--
Best regards
Japin Li



stream-replication-protocol-documentation.patch
Description: stream-replication-protocol-documentation.patch

Re: Confused about stream replication protocol documentation

[Li Japin]

On Dec 22, 2020, at 11:13 PM, Fujii Masao 
mailto:masao.fu...@oss.nttdata.com>> wrote:

‘B’ means a backend and ‘F’ means a frontend. Maybe as [1] does, we should
add the note like "Each is marked to indicate that it can be sent by
a frontend (F) and a backend (B)" into the description about each message
format for START_REPLICATION.

[1]
https://www.postgresql.org/docs/devel/protocol-message-formats.html

Thanks for your clarify.  Maybe we should move the "protocol message formats”
before “stream replication protocol” or referenced it in "stream replication 
protocol”.

--
Best regards
Japin Li

Confused about stream replication protocol documentation

[Li Japin]

Hi, all

In Stream Replication Protocol [1], the documentation of `START_REPLICATION` 
message is

XLogData (B)
  …
Primary keepalive message (B)
  …
Standby status update (F)
  …
Hot Standby feedback message (F)
  ...

I’m confused about the means of ‘B’ and ‘F’? If it doesn't make sense, why we 
document here?
However, if it makes sense, should we explain it?
Can someone help me out?

Anyway, thanks in advance!

[1] https://www.postgresql.org/docs/devel/protocol-replication.html

--
Best regards
Japin Li

Re: Feature improvement for pg_stat_statements

[Li Japin]

Hi,

On Dec 9, 2020, at 6:37 PM, Seino Yuki 
mailto:sein...@oss.nttdata.com>> wrote:

2020-12-01 01:04 に Fujii Masao さんは書きました:
On 2020/11/30 23:05, Seino Yuki wrote:
2020-11-30 15:02 に Fujii Masao さんは書きました:
On 2020/11/30 12:08, Seino Yuki wrote:
2020-11-27 22:39 に Fujii Masao さんは書きました:
On 2020/11/27 21:39, Seino Yuki wrote:
2020-11-27 21:37 に Seino Yuki さんは書きました:
2020-11-16 12:28 に Seino Yuki さんは書きました:
Due to similar fixed, we have also merged the patches discussed in the
following thread.
https://commitfest.postgresql.org/30/2736/
The patch is posted on the 2736 side of the thread.
Regards.
I forgot the attachment and will resend it.
The following patches have been committed and we have created a
compliant patch for them.
https://commitfest.postgresql.org/30/2736/
Please confirm.
Thanks for updating the patch! Here are review comments from me.
+OUT reset_exec_time TIMESTAMP WITH TIME ZONE
I prefer "stats_reset" as the name of this column for the sake of
consistency with the similar column in other stats views like
pg_stat_database.
Isn't it better to use lower cases for "TIMESTAMP WITH TIME ZONE"
because other DDLs in pg_stat_statements do that for the declaraion
of data type?
@@ -565,6 +568,8 @@ pgss_shmem_startup(void)
 pgss->n_writers = 0;
 pgss->gc_count = 0;
 pgss->stats.dealloc = 0;
+pgss->stats.reset_exec_time = 0;
+pgss->stats.reset_exec_time_isnull = true;
The reset time should be initialized with GetCurrentTimestamp() instead
of 0? If so, I think that we can completely get rid of reset_exec_time_isnull.
+memset(nulls, 0, sizeof(nulls));
If some entries in "values[]" may not be set, "values[]" also needs to
be initialized with 0.
MemSet() should be used, instead?
+/* Read dealloc */
+values[0] = stats.dealloc;
Int64GetDatum() should be used here?
+reset_ts = GetCurrentTimestamp();
GetCurrentTimestamp() needs to be called only when all the entries
are removed. But it's called even in other cases.
Regards,
Thanks for the review!
Fixed the patch.
Thanks for updating the patch! Here are another review comments.
+   reset_exec_time timestamp
with time zone
You forgot to update the column name in the doc?
+Shows the time at which
pg_stat_statements_reset(0,0,0) was last called.
What about updating this to something like "Time at which all statistics
in the pg_stat_statements view were last reset." for the sale of
onsistency with the description about stats_reset column in other
tats views?
+/* Read stats_reset */
+values[1] = stats.stats_reset;
TimestampTzGetDatum() seems necessary.
+reset_ts = GetCurrentTimestamp();
 /* Reset global statistics for pg_stat_statements */
Isn't it better to call GetCurrentTimestamp() before taking
an exclusive lwlock, in entry_reset()?
Regards,
Thanks for the new comment.
I got the following pointers earlier.
+reset_ts = GetCurrentTimestamp();
GetCurrentTimestamp() needs to be called only when all the entries
are removed. But it's called even in other cases.
Which do you think is better? I think the new pointing out is better,
because entry_reset is not likely to be called often.
I was thinking that GetCurrentTimestamp() should be called before
pgss->lock lwlock is taken, only when all three arguments userid, dbid
and queryid are zero. But on second thought, we should call
GetCurrentTimestamp() and reset the stats, after the following codes?
/* All entries are removed? */
if (num_entries != num_remove)
goto release_lock;
That is, IMO that even when pg_stat_statements_reset() with non-zero
arguments is executed, if it removes all the entries, we should reset
the stats. Thought?
Regards,

+1.Fixed the patch.
We tested various reset patterns and they seemed to be fine.


Since BlessTupleDesc() is for SRFs according to the comments, I think, we can
remove it here.  Correct?

+   if (get_call_result_type(fcinfo, NULL, ) != TYPEFUNC_COMPOSITE)
+   elog(ERROR, "return type must be a row type");
+
+   tupdesc = BlessTupleDesc(tupdesc);

--
Best regards
ChengDu WenWu Information Technology Co.,Ltd.
Japin Li

Re: About to add WAL write/fsync statistics to pg_stat_wal view

[Li Japin]

Hi,

> On Dec 8, 2020, at 1:06 PM, Masahiro Ikeda  wrote:
> 
> Hi,
> 
> I propose to add wal write/fsync statistics to pg_stat_wal view.
> It's useful not only for developing/improving source code related to WAL
> but also for users to detect workload changes, HW failure, and so on.
> 
> I introduce "track_wal_io_timing" parameter and provide the following 
> information on pg_stat_wal view.
> I separate the parameter from "track_io_timing" to "track_wal_io_timing"
> because IIUC, WAL I/O activity may have a greater impact on query performance 
> than database I/O activity.
> 
> ```
> postgres=# SELECT wal_write, wal_write_time, wal_sync, wal_sync_time FROM 
> pg_stat_wal;
> -[ RECORD 1 ]--+
> wal_write  | 650  # Total number of times WAL data was written to the disk
> 
> wal_write_time | 43   # Total amount of time that has been spent in the 
> portion of WAL data was written to disk
>  # if track-wal-io-timing is enabled, otherwise zero
> 
> wal_sync   | 78   # Total number of times WAL data was synced to the disk
> 
> wal_sync_time  | 104  # Total amount of time that has been spent in the 
> portion of WAL data was synced to disk
>  # if track-wal-io-timing is enabled, otherwise zero
> ```
> 
> What do you think?
> Please let me know your comments.
> 
> Regards
> -- 
> Masahiro Ikeda
> NTT DATA CORPORATION<0001_add_wal_io_activity_to_the_pg_stat_wal.patch>

There is a no previous prototype warning for ‘fsyncMethodCalled’, and it now 
only used in xlog.c,
should we declare with static? And this function wants a boolean as a return, 
should we use
true/false other than 0/1?

+/*
+ * Check if fsync mothod is called.
+ */
+bool
+fsyncMethodCalled()
+{
+   if (!enableFsync)
+   return 0;
+
+   switch (sync_method)
+   {
+   case SYNC_METHOD_FSYNC:
+   case SYNC_METHOD_FSYNC_WRITETHROUGH:
+   case SYNC_METHOD_FDATASYNC:
+   return 1;
+   default:
+   /* others don't have a specific fsync method */
+   return 0;
+   }
+}
+

--
Best regards
ChengDu WenWu Information Technology Co.,Ltd.
Japin Li

Re: Printing LSN made easy

[Li Japin]

Hi,

On Nov 30, 2020, at 9:06 PM, Ashutosh Bapat 
mailto:ashutosh.bapat@gmail.com>> wrote:

On Fri, Nov 27, 2020 at 9:51 PM Li Japin 
mailto:japi...@hotmail.com>> wrote:

Hi,

Here, we cannot use sizeof(but) to get the buf size, because it is a pointer, 
so it always
8 bytes on 64-bit or 4 bytes on 32-bit machine.

For an array, the sizeof() returns the size of memory consumed by the
array. See section "Application to arrays" at
https://en.wikipedia.org/wiki/Sizeof.

That’s true! However, in pg_lsn_out_buffer(), it converts to a pointer, not an 
array. See the following test:

```c
#include 
#include 

typedef uint64_tXLogRecPtr;
typedef uint32_tuint32;

#define MAXPG_LSNLEN17
#define LSN_FORMAT "%X/%X"
#define LSN_FORMAT_ARG(lsn) (uint32) ((lsn) >> 32), (uint32) (lsn)

char *
pg_lsn_out_buffer(XLogRecPtr lsn, char *buf)
{
  printf("pg_lsn_out_buffer: sizeof(buf) = %lu\n", sizeof(buf));
  snprintf(buf, sizeof(buf), LSN_FORMAT, LSN_FORMAT_ARG(lsn));
  return buf;
}

char *
pg_lsn_out_buffer1(XLogRecPtr lsn, char *buf, size_t len)
{
  printf("pg_lsn_out_buffer1: sizeof(buf) = %lu, len = %lu\n", sizeof(buf), 
len);
  snprintf(buf, len, LSN_FORMAT, LSN_FORMAT_ARG(lsn));
  return buf;
}

int
main(void)
{
  char buf[MAXPG_LSNLEN + 1];
  XLogRecPtrlsn = 1234567UL;

  printf("main: sizeof(buf) = %lu\n", sizeof(buf));
  pg_lsn_out_buffer(lsn, buf);
  printf("buffer's content from pg_lsn_out_buffer: %s\n", buf);

  pg_lsn_out_buffer1(lsn, buf, sizeof(buf));
  printf("buffer's content from pg_lsn_out_buffer1: %s\n", buf);
  return 0;
}
```

The above output is:

```
main: sizeof(buf) = 18
pg_lsn_out_buffer: sizeof(buf) = 8
buffer's content from pg_lsn_out_buffer: 0/12D68
pg_lsn_out_buffer1: sizeof(buf) = 8, len = 18
buffer's content from pg_lsn_out_buffer1: 0/12D687
```

--
Best regards
Japin Li
ChengDu WenWu Information Technolog Co.,Ltd.

Re: Printing LSN made easy

[Li Japin]

Hi,

Here, we cannot use sizeof(but) to get the buf size, because it is a pointer, 
so it always
8 bytes on 64-bit or 4 bytes on 32-bit machine.

+char *
+pg_lsn_out_buffer(XLogRecPtr lsn, char *buf)
+{
+   snprintf(buf, sizeof(buf), LSN_FORMAT, LSN_FORMAT_ARG(lsn));
+
+   return buf;
+}

--
Best regards
Japin Li
ChengDu WenWu Information Technolog Co.,Ltd.


> On Nov 27, 2020, at 10:24 PM, Alexey Kondratov  
> wrote:
> 
> Hi,
> 
> On 2020-11-27 13:40, Ashutosh Bapat wrote:
>> Off list Peter Eisentraut pointed out that we can not use these macros
>> in elog/ereport since it creates problems for translations. He
>> suggested adding functions which return strings and use %s when doing
>> so.
>> The patch has two functions pg_lsn_out_internal() which takes an LSN
>> as input and returns a palloc'ed string containing the string
>> representation of LSN. This may not be suitable in performance
>> critical paths and also may leak memory if not freed. So there's
>> another function pg_lsn_out_buffer() which takes LSN and a char array
>> as input, fills the char array with the string representation and
>> returns the pointer to the char array. This allows the function to be
>> used as an argument in printf/elog etc. Macro MAXPG_LSNLEN has been
>> extern'elized for this purpose.
> 
> If usage of macros in elog/ereport can cause problems for translation, then 
> even with this patch life is not get simpler significantly. For example, 
> instead of just doing like:
> 
> elog(WARNING,
> - "xlog min recovery request %X/%X is past current point 
> %X/%X",
> - (uint32) (lsn >> 32), (uint32) lsn,
> - (uint32) (newMinRecoveryPoint >> 32),
> - (uint32) newMinRecoveryPoint);
> + "xlog min recovery request " LSN_FORMAT " is past current 
> point " LSN_FORMAT,
> + LSN_FORMAT_ARG(lsn),
> + LSN_FORMAT_ARG(newMinRecoveryPoint));
> 
> we have to either declare two additional local buffers, which is verbose; or 
> use pg_lsn_out_internal() and rely on memory contexts (or do pfree() 
> manually, which is verbose again) to prevent memory leaks.
> 
>> Off list Craig Ringer suggested introducing a new format specifier
>> similar to %m for LSN but I did not get time to take a look at the
>> relevant code. AFAIU it's available only to elog/ereport, so may not
>> be useful generally. But teaching printf variants about the new format
>> would be the best solution. However, I didn't find any way to do that.
> 
> It seems that this topic has been extensively discussed off-list, but still 
> strong +1 for the patch. I always wanted LSN printing to be more concise.
> 
> I have just tried new printing utilities in a couple of new places and it 
> looks good to me.
> 
> +char *
> +pg_lsn_out_internal(XLogRecPtr lsn)
> +{
> + charbuf[MAXPG_LSNLEN + 1];
> +
> + snprintf(buf, sizeof(buf), LSN_FORMAT, LSN_FORMAT_ARG(lsn));
> +
> + return pstrdup(buf);
> +}
> 
> Would it be a bit more straightforward if we palloc buf initially and just 
> return a pointer instead of doing pstrdup()?
> 
> 
> Regards
> -- 
> Alexey Kondratov
> 
> Postgres Professional https://www.postgrespro.com
> Russian Postgres Company<0001-Make-it-easy-to-print-LSN.patch>

Re: Terminate the idle sessions

[Li Japin]

On Nov 24, 2020, at 11:20 PM, David G. Johnston 
mailto:david.g.johns...@gmail.com>> wrote:

On Mon, Nov 23, 2020 at 11:22 PM Li Japin 
mailto:japi...@hotmail.com>> wrote:

How about use “foreign-data wrapper” replace “postgres_fdw”?

I don't see much value in avoiding mentioning that specific term - my proposal 
turned it into an example instead of being exclusive.


- This parameter should be set to zero if you use some 
connection-pooling software,
- or pg servers used by postgres_fdw, because connections might be 
closed unexpectedly.
+ This parameter should be set to zero if you use connection-pooling 
software,
+ or PostgreSQL servers connected to using 
foreign-data
+ wrapper, because connections might be closed unexpectedly.
 

Maybe:

+ or your PostgreSQL server receives connection from postgres_fdw or similar 
middleware.
+ Such software is expected to self-manage its connections.

Thank you for your suggestion and patient! Fixed.

```
+
+ This parameter should be set to zero if you use connection-pooling 
software,
+ or PostgreSQL servers connected to using 
postgres_fdw
+ or similar middleware (such software is expected to self-manage its 
connections),
+ because connections might be closed unexpectedly.
+
```

--
Best regards
Japin Li



v9-0001-Allow-terminating-the-idle-sessions.patch
Description: v9-0001-Allow-terminating-the-idle-sessions.patch


v9-0002-Optimize-setitimer-usage.patch
Description: v9-0002-Optimize-setitimer-usage.patch

Remove cache_plan argument comments to ri_PlanCheck

[Li Japin]

Hi, hackers

I found that the cache_plan argument to ri_PlanCheck already been remove since
5b7ba75f7ff854003231e8099e3038c7e2eba875.   I think we can remove the comments
tor cache_plan to ri_PlanCheck.

diff --git a/src/backend/utils/adt/ri_triggers.c 
b/src/backend/utils/adt/ri_triggers.c
index 7e2b2e3dd6..02b1a3868f 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2130,9 +2130,6 @@ InvalidateConstraintCacheCallBack(Datum arg, int cacheid, 
uint32 hashvalue)

 /*
  * Prepare execution plan for a query to enforce an RI restriction
- *
- * If cache_plan is true, the plan is saved into our plan hashtable
- * so that we don't need to plan it again.
  */
 static SPIPlanPtr
 ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes,

--
Best regards
Japin Li
ChengDu WenWu Information Technology Co.,Ltd.



cache_plan-to-ri_PlanCheck.diff
Description: cache_plan-to-ri_PlanCheck.diff

Re: Use macros for calculating LWLock offset

[Li Japin]

Thanks!

On Nov 24, 2020, at 11:51 AM, Michael Paquier 
mailto:mich...@paquier.xyz>> wrote:

On Fri, Nov 20, 2020 at 03:25:50PM +0900, Michael Paquier wrote:
I agree that this makes this code a bit cleaner, so let's use those
macros.  Others may have some comments here, so let's wait a bit
first.

Got this one committed as of d03d754.
—
Michael

--
Best regards
Japin Li

Re: Terminate the idle sessions

[Li Japin]

Hi, David

Thanks for your suggestion!

On Nov 24, 2020, at 11:39 AM, David G. Johnston 
mailto:david.g.johns...@gmail.com>> wrote:

On Mon, Nov 23, 2020 at 5:02 PM 
kuroda.hay...@fujitsu.com 
mailto:kuroda.hay...@fujitsu.com>> wrote:
No one have any comments, patch tester says OK, and I think this works well.
I changed status to "Ready for Committer."
Some proof-reading:

v8-0001

Documentation:

My suggestion wasn't taken for the first note paragraph (review/author 
disagreement) and the current has the following issues:

Sorry for ignoring this suggestion.

"if you use some connection-pooling" software doesn't need the word "some"
Don't substitute "pg" for the name of the product, PostgreSQL.
The word "used" is a more stylistic dislike, but "connected to using 
postgres_fdw" would be a better choice IMO.

Code (minor, but if you are in there anyway):


How about use “foreign-data wrapper” replace “postgres_fdw”?

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b71a116be3..a3a50e7bdb 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8293,8 +8293,9 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH 
csv;


 
- This parameter should be set to zero if you use some 
connection-pooling software,
- or pg servers used by postgres_fdw, because connections might be 
closed unexpectedly.
+ This parameter should be set to zero if you use connection-pooling 
software,
+ or PostgreSQL servers connected to using 
foreign-data
+ wrapper, because connections might be closed unexpectedly.
 
 
  Aside from a bit of resource consumption idle sessions do not 
interfere with the

(5) turn off ... timeout (there are now two, timeouts should be plural)

Fixed.

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index ba2369b72d..bcf8c610fd 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4278,7 +4278,7 @@ PostgresMain(int argc, char *argv[],
DoingCommandRead = false;

/*
-* (5) turn off the idle-in-transaction and idle-session timeout
+* (5) turn off the idle-in-transaction and idle-session 
timeouts
 */
if (disable_idle_in_transaction_timeout)
{


I will send a new patch if there is not other comments.

--
Best Regards,
Japin Li

Re: Terminate the idle sessions

[Li Japin]

Hi, Kuroda

Thank for your review.

> On Nov 24, 2020, at 8:01 AM, kuroda.hay...@fujitsu.com wrote:
> 
> No one have any comments, patch tester says OK, and I think this works well.
> I changed status to "Ready for Committer."
> 
> Hayato Kuroda
> FUJITSU LIMITED
> 
> -Original Message-
> From: kuroda.hay...@fujitsu.com  
> Sent: Friday, November 20, 2020 11:05 AM
> To: 'japin' 
> Cc: David G. Johnston ; Kyotaro Horiguchi 
> ; Thomas Munro ; 
> bharath.rupireddyforpostg...@gmail.com; pgsql-hackers@lists.postgresql.org
> Subject: RE: Terminate the idle sessions
> 
> Dear Li,
> 
>> Thanks! Add the comment for idle-session timeout.
> 
> I confirmed it. OK.
> 
> 
> I don't have any comments anymore. If no one has,
> I will change the status few days later.
> Other comments or suggestions to him?
> 
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
> 

--
Best regards
Japin Li

Re: Terminate the idle sessions

[Li Japin]

On Nov 18, 2020, at 2:22 PM, 
kuroda.hay...@fujitsu.com wrote:

Oops.. I forgot putting my suggestion. Sorry.
How about substituting sigalrm_delivered to true in the reschedule_timeouts()?
Maybe this processing looks strange, so some comments should be put too.
Here is an example:

```diff
@@ -423,7 +423,14 @@ reschedule_timeouts(void)

   /* Reschedule the interrupt, if any timeouts remain active. */
   if (num_active_timeouts > 0)
+   {
+   /*
+* sigalrm_delivered is set to true,
+* because any intrreputions might be occured.
+*/
+   sigalrm_delivered = true;
   schedule_alarm(GetCurrentTimestamp());
+   }
}
```

Thanks for your suggestion.  Attached!

--
Best regards
Japin Li



v7-0001-Allow-terminating-the-idle-sessions.patch
Description: v7-0001-Allow-terminating-the-idle-sessions.patch


v7-0002-Optimize-setitimer-usage.patch
Description: v7-0002-Optimize-setitimer-usage.patch

Re: Terminate the idle sessions

[Li Japin]

On Nov 18, 2020, at 10:40 AM, 
kuroda.hay...@fujitsu.com wrote:


I’m not familiar with the system interrupt, however,
the sigalrm_due_at is subsutitue between HOLD_INTERRUPTS()
and RESUM_INTERRUPTS(), so I think it cannot be interrupted.
The following comments comes from miscadmin.h.

Right, but how about before HOLD_INTERRUPTS()?
If so, only calling handle_sig_alarm() is occurred, and
Setitimer will not be set, I think.

Yeah, it might be occurred. Any suggestions to fix it?

--
Best regards
Japin Li

Re: Terminate the idle sessions

[Li Japin]

On Nov 17, 2020, at 2:07 PM, 
kuroda.hay...@fujitsu.com wrote:

Dear Li, David,

> Additionally, using postgres_fdw within the server doesn't cause issues,
> its using postgres_fdw and the remote server having this setting set to zero 
> that causes a problem.

I didn't know the fact that postgres_fdw can use within the server... Thanks.

I read optimize-setitimer patch, and looks basically good. I put what I 
understanding,
so please confirm it whether your implementation is correct.
(Maybe I missed some simultaneities, so please review anyone...)

[besic consept]

sigalrm_due_at means the time that interval timer will ring, and 
sigalrm_delivered means who calls schedule_alarm().
If fin_time of active_timeouts[0] is larger than or equal to sigalrm_due_at,
stop calling setitimer because handle_sig_alarm() will be call sooner.


Agree. The sigalrm_delivered means a timer has been handled by 
handle_sig_alarm(), so we should call setitimer() in next schedule_alarm().

[when call setitimer]

In the attached patch, setitimer() will be only called the following scenarios:

* when handle_sig_alarm() is called due to the pqsignal
* when a timeout is registered and its fin_time is later than active_timeous[0]
* when disable a timeout
* when handle_sig_alarm() is interrupted and rescheduled(?)

According to comments, handle_sig_alarm() may be interrupted because of the 
ereport.
I think if handle_sig_alarm() is interrupted before subsutituting 
sigalrm_due_at to true,
interval timer will be never set. Is it correct, or is my assumption wrong?


I’m not familiar with the system interrupt, however, the sigalrm_due_at is 
subsutitue between HOLD_INTERRUPTS()
and RESUM_INTERRUPTS(), so I think it cannot be interrupted.  The following 
comments comes from miscadmin.h.

> The HOLD_INTERRUPTS() and RESUME_INTERRUPTS() macros
> allow code to ensure that no cancel or die interrupt will be accepted,
> even if CHECK_FOR_INTERRUPTS() gets called in a subroutine.  The interrupt
> will be held off until CHECK_FOR_INTERRUPTS() is done outside any
> HOLD_INTERRUPTS() ... RESUME_INTERRUPTS() section.


Lastly, I found that setitimer is obsolete and should change to another one. 
According to my man page:

```
POSIX.1-2001, SVr4, 4.4BSD (this call first appeared in 4.2BSD).
POSIX.1-2008 marks getitimer() and setitimer() obsolete,
recommending the use of the POSIX timers API (timer_gettime(2), 
timer_settime(2), etc.) instead.
```

Do you have an opinion for this? I think it should be changed
if all platform can support timer_settime system call, but this fix affects all 
timeouts,
so more considerations might be needed.


Not sure! I find that Win32 do not support setitimer(), PostgreSQL emulate 
setitimer() by creating a persistent thread to handle
the timer setting and notification upon timeout.

So if we want to replace it, I think we should open a new thread to achieve 
this.

--
Best regards
Japin Li

Re: Terminate the idle sessions

[Li Japin]

On Nov 17, 2020, at 10:53 AM, David G. Johnston 
mailto:david.g.johns...@gmail.com>> wrote:

On Monday, November 16, 2020, Li Japin 
mailto:japi...@hotmail.com>> wrote:


Consider setting this for specific users instead of as a server default.  
Client connections managed by connection poolers, or initiated indirectly like 
those by a remote postgres_fdw  using server, should probably be excluded from 
this timeout.


 
- This parameter should be set to zero if you use postgres_fdw or some
- connection-pooling software, because connections might be closed 
unexpectedly.
+ This parameter should be set to zero if you use some 
connection-pooling software, or
+ PostgreSQL servers used by postgres_fdw, because connections might be 
closed unexpectedly.
 



Prefer mine, “or pg servers used by postgres_fdw”, doesn’t flow.

Could you please explain how the idle-in-transaction interfere the long-running 
stability?

From the docs (next section):

This allows any locks held by that session to be released and the connection 
slot to be reused; it also allows tuples visible only to this transaction to be 
vacuumed. See Section 
24.1<https://www.postgresql.org/docs/13/routine-vacuuming.html> for more 
details about this.

Thanks David! Attached.

--
Best regards
Japin Li


v6-0001-Allow-terminating-the-idle-sessions.patch
Description: v6-0001-Allow-terminating-the-idle-sessions.patch


v6-0002-Optimize-setitimer-usage.patch
Description: v6-0002-Optimize-setitimer-usage.patch

Re: Terminate the idle sessions

[Li Japin]

--
Best regards
Japin Li

On Nov 17, 2020, at 7:59 AM, David G. Johnston 
mailto:david.g.johns...@gmail.com>> wrote:

On Mon, Nov 16, 2020 at 5:41 AM Li Japin 
mailto:japi...@hotmail.com>> wrote:
Thanks for your review! Attached.

Reading the doc changes:

I'd rather not name postgres_fdw explicitly, or at least not solely, as a 
reason for setting this to zero.  Additionally, using postgres_fdw within the 
server doesn't cause issues, its using postgres_fdw and the remote server 
having this setting set to zero that causes a problem.


Consider setting this for specific users instead of as a server default.  
Client connections managed by connection poolers, or initiated indirectly like 
those by a remote postgres_fdw using server, should probably be excluded from 
this timeout.

Text within  should be indented one space (you missed both under 
listitem).

Thanks for your suggest! How about change document as follows:

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6c4e2a1fdc..23e691a7c5 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8281,17 +8281,17 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH 
csv;
   
   

-   Terminate any session that has been idle for longer than the specified 
amount of time.
+Terminate any session that has been idle for longer than the specified 
amount of time.


-   If this value is specified without units, it is taken as milliseconds.
-   A value of zero (the default) disables the timeout.
+If this value is specified without units, it is taken as milliseconds.
+A value of zero (the default) disables the timeout.



 
- This parameter should be set to zero if you use postgres_fdw or some
- connection-pooling software, because connections might be closed 
unexpectedly.
+ This parameter should be set to zero if you use some 
connection-pooling software, or
+ PostgreSQL servers used by postgres_fdw, because connections might be 
closed unexpectedly.
 


I'd suggest a comment that aside from a bit of resource consumption idle 
sessions do not interfere with the long-running stability of the server, unlike 
idle-in-transaction sessions which are controlled by the other configuration 
setting.

Could you please explain how the idle-in-transaction interfere the long-running 
stability?

--
Best regards
Japin Li

Re: Terminate the idle sessions

[Li Japin]

Hi Kuroda,

On Nov 16, 2020, at 1:22 PM, 
kuroda.hay...@fujitsu.com wrote:


@@ -30,6 +30,7 @@ typedef enum TimeoutId
STANDBY_DEADLOCK_TIMEOUT,
STANDBY_TIMEOUT,
STANDBY_LOCK_TIMEOUT,
+ IDLE_SESSION_TIMEOUT,
IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
/* First user-definable timeout reason */
USER_TIMEOUT,

I'm not familiar with timeout, but I can see that the priority of idle-session 
is set lower than transaction-timeout.
Could you explain the reason? In my image this timeout locates at the lowest 
layer, so it might have the lowest
priority.

My apologies! I just add a enum for idle session and ignore the comments that 
says the enum has priority.
Fixed as follows:

@@ -30,8 +30,8 @@ typedef enum TimeoutId
STANDBY_DEADLOCK_TIMEOUT,
STANDBY_TIMEOUT,
STANDBY_LOCK_TIMEOUT,
-   IDLE_SESSION_TIMEOUT,
IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
+   IDLE_SESSION_TIMEOUT,
/* First user-definable timeout reason */
USER_TIMEOUT,
/* Maximum number of timeout reasons */

Thanks for your review! Attached.

--
Best regards
Japin Li






v5-0001-Allow-terminating-the-idle-sessions.patch
Description: v5-0001-Allow-terminating-the-idle-sessions.patch


v5-0002-Optimize-setitimer-usage.patch
Description: v5-0002-Optimize-setitimer-usage.patch

Re: Terminate the idle sessions

[Li Japin]

On Nov 13, 2020, at 6:27 PM, 
kuroda.hay...@fujitsu.com wrote:


I read your patch, and I think the documentation is too simple to avoid all 
problems.
(I think if some connection pooling is used, the same problem will occur.)
Could you add some explanations in the doc file? I made an example:

```
Note that this values should be set to zero if you use postgres_fdw or some
Connection-pooling software, because connections might be closed unexpectedly.
```

Thanks for your advice! Attached v4.

--
Best regards
Japin Li



v4-0001-Allow-terminating-the-idle-sessions.patch
Description: v4-0001-Allow-terminating-the-idle-sessions.patch


v4-0002-Optimize-setitimer-usage.patch
Description: v4-0002-Optimize-setitimer-usage.patch

Re: Tab complete for alter table rls

[Li Japin]

Thanks Michael!

--
Best regards
Japin Li



> On Oct 24, 2020, at 9:49 AM, Michael Paquier  wrote:
> 
> On Fri, Oct 23, 2020 at 04:37:18PM +0900, Michael Paquier wrote:
>> No worries.  Good catch.  I'll try to test that and apply it later,
>> but by reading the code it looks like you got that right.
> 
> Checked and applied on HEAD, thanks!
> --
> Michael

Tab complete for alter table rls

[Li Japin]

Sorry, I forgot add the subject.

--
Best regards
Japin Li

On Oct 23, 2020, at 1:19 PM, Li Japin 
mailto:japi...@hotmail.com>> wrote:

Hi, hackers

I find that ALTER TABLE xxx FORCE/NO FORCE ROW LEVEL SECURITY cannot support 
tab complete.
The attached add the tab complete for rls.

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 561fe1dff9..b2b4f1fd4d 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1974,10 +1974,10 @@ psql_completion(const char *text, int start, int end)
  */
  else if (Matches("ALTER", "TABLE", MatchAny))
  COMPLETE_WITH("ADD", "ALTER", "CLUSTER ON", "DISABLE", "DROP",
-  "ENABLE", "INHERIT", "NO INHERIT", "RENAME", "RESET",
+  "ENABLE", "INHERIT", "NO", "RENAME", "RESET",
   "OWNER TO", "SET", "VALIDATE CONSTRAINT",
   "REPLICA IDENTITY", "ATTACH PARTITION",
-  "DETACH PARTITION");
+  "DETACH PARTITION", "FORCE ROW LEVEL SECURITY");
  /* ALTER TABLE xxx ENABLE */
  else if (Matches("ALTER", "TABLE", MatchAny, "ENABLE"))
  COMPLETE_WITH("ALWAYS", "REPLICA", "ROW LEVEL SECURITY", "RULE",
@@ -2007,6 +2007,9 @@ psql_completion(const char *text, int start, int end)
  /* ALTER TABLE xxx INHERIT */
  else if (Matches("ALTER", "TABLE", MatchAny, "INHERIT"))
  COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, "");
+ /* ALTER TABLE xxx NO */
+ else if (Matches("ALTER", "TABLE", MatchAny, "NO"))
+ COMPLETE_WITH("FORCE ROW LEVEL SECURITY", "INHERIT");
  /* ALTER TABLE xxx NO INHERIT */
  else if (Matches("ALTER", "TABLE", MatchAny, "NO", "INHERIT"))
  COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, "”);

Best regards.


--
ChengDu WenWu Information Technology Co,Ltd.
Japin Li

<0001-Add-tab-complete-for-alter-table-rls.patch>

[no subject]

[Li Japin]

Hi, hackers

I find that ALTER TABLE xxx FORCE/NO FORCE ROW LEVEL SECURITY cannot support 
tab complete.
The attached add the tab complete for rls.

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 561fe1dff9..b2b4f1fd4d 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1974,10 +1974,10 @@ psql_completion(const char *text, int start, int end)
  */
  else if (Matches("ALTER", "TABLE", MatchAny))
  COMPLETE_WITH("ADD", "ALTER", "CLUSTER ON", "DISABLE", "DROP",
-  "ENABLE", "INHERIT", "NO INHERIT", "RENAME", "RESET",
+  "ENABLE", "INHERIT", "NO", "RENAME", "RESET",
   "OWNER TO", "SET", "VALIDATE CONSTRAINT",
   "REPLICA IDENTITY", "ATTACH PARTITION",
-  "DETACH PARTITION");
+  "DETACH PARTITION", "FORCE ROW LEVEL SECURITY");
  /* ALTER TABLE xxx ENABLE */
  else if (Matches("ALTER", "TABLE", MatchAny, "ENABLE"))
  COMPLETE_WITH("ALWAYS", "REPLICA", "ROW LEVEL SECURITY", "RULE",
@@ -2007,6 +2007,9 @@ psql_completion(const char *text, int start, int end)
  /* ALTER TABLE xxx INHERIT */
  else if (Matches("ALTER", "TABLE", MatchAny, "INHERIT"))
  COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, "");
+ /* ALTER TABLE xxx NO */
+ else if (Matches("ALTER", "TABLE", MatchAny, "NO"))
+ COMPLETE_WITH("FORCE ROW LEVEL SECURITY", "INHERIT");
  /* ALTER TABLE xxx NO INHERIT */
  else if (Matches("ALTER", "TABLE", MatchAny, "NO", "INHERIT"))
  COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, "”);

Best regards.


--
ChengDu WenWu Information Technology Co,Ltd.
Japin Li



0001-Add-tab-complete-for-alter-table-rls.patch
Description: 0001-Add-tab-complete-for-alter-table-rls.patch

Re: Enumize logical replication message actions

[Li Japin]

> On Oct 16, 2020, at 3:25 PM, Ashutosh Bapat  
> wrote:
> 
> Hi All,
> Logical replication protocol uses single byte character to identify
> different chunks of logical repliation messages. The code uses
> character literals for the same. These literals are used as bare
> constants in code as well. That's true for almost all the code that
> deals with wire protocol. With that it becomes difficult to identify
> the code which deals with a particular message. For example code that
> deals with message type 'B'. In various protocol 'B' has different
> meaning and it gets difficult and time consuming to differentiate one
> usage from other and find all places which deal with one usage. Here's
> a patch simplifying that for top level logical replication messages.
> 
> I think I have covered the places that need change. But I might have
> missed something, given that these literals are used at several other
> places (a problem this patch tries to fix :)).
> 
> Initially I had used #define for the same, but Peter E suggested using
> Enums so that switch cases can detect any remaining items along with
> stronger type checks.
> 
> Pavan offleast suggested to create a wrapper
> pg_send_logical_rep_message() on top of pg_sendbyte(), similarly for
> pg_getmsgbyte(). I wanted to see if this change is acceptable. If so,
> I will change that as well. Comments/suggestions welcome.
> 
> -- 
> Best Wishes,
> Ashutosh Bapat
> <0001-Enumize-top-level-logical-replication-actions.patch>

What about ’N’ for new tuples, ‘O’ for old tuple follows, ‘K’ for old key 
follows?
Those are also logical replication protocol message, I think.

--
Best regards
Japin Li

Re: Remove unnecessary else branch

[Li Japin]

On Oct 13, 2020, at 9:59 PM, Hamid Akhtar 
mailto:hamid.akh...@gmail.com>> wrote:



On Tue, Oct 13, 2020 at 6:37 PM Heikki Linnakangas 
mailto:hlinn...@iki.fi>> wrote:
On 13/10/2020 16:30, Li Japin wrote:
> Hi,
>
> I found in guc-file.l we can omit the else branch in AbsoluteConfigLocation().

It will compile the same, so it's just a matter of code readability or
taste which style is better here. I think we should leave it alone, it's
fine as it is.

- Heikki



I agree with Heikki from the code execution point of view.

In code execution point of view they are same, however, the code is for user, i 
think the readability is also important.


"canonicalize_path(abs_path);" statement is also condition independent and can 
be pulled out of both if and else blocks. Removing unnecessary statements makes 
the code more readable, but it is a matter of choice/style.

+1

diff --git a/src/backend/utils/misc/guc-file.l 
b/src/backend/utils/misc/guc-file.l
index c98e220295..b3549665ef 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -522,23 +522,21 @@ AbsoluteConfigLocation(const char *location, const char 
*calling_file)

if (is_absolute_path(location))
return pstrdup(location);
+
+   if (calling_file != NULL)
+   {
+   strlcpy(abs_path, calling_file, sizeof(abs_path));
+   get_parent_directory(abs_path);
+   join_path_components(abs_path, abs_path, location);
+   }
else
{
-   if (calling_file != NULL)
-   {
-   strlcpy(abs_path, calling_file, sizeof(abs_path));
-   get_parent_directory(abs_path);
-   join_path_components(abs_path, abs_path, location);
-   canonicalize_path(abs_path);
-   }
-   else
-   {
-   AssertState(DataDir);
-   join_path_components(abs_path, DataDir, location);
-   canonicalize_path(abs_path);
-   }
-   return pstrdup(abs_path);
+   AssertState(DataDir);
+   join_path_components(abs_path, DataDir, location);
}
+
+   canonicalize_path(abs_path);
+   return pstrdup(abs_path);
 }

--
Best regards
Japin Li

Re: Remove unnecessary else branch

[Li Japin]

On Oct 13, 2020, at 9:59 PM, Hamid Akhtar 
mailto:hamid.akh...@gmail.com>> wrote:



On Tue, Oct 13, 2020 at 6:37 PM Heikki Linnakangas 
mailto:hlinn...@iki.fi>> wrote:
On 13/10/2020 16:30, Li Japin wrote:
> Hi,
>
> I found in guc-file.l we can omit the else branch in AbsoluteConfigLocation().

It will compile the same, so it's just a matter of code readability or
taste which style is better here. I think we should leave it alone, it's
fine as it is.

- Heikki



I agree with Heikki from the code execution point of view.

In code execution point of view they are same, however, the code is for user, i 
think the readability is also important.


"canonicalize_path(abs_path);" statement is also condition independent and can 
be pulled out of both if and else blocks. Removing unnecessary statements makes 
the code more readable, but it is a matter of choice/style.

+1

diff --git a/src/backend/utils/misc/guc-file.l 
b/src/backend/utils/misc/guc-file.l
index c98e220295..b3549665ef 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -522,23 +522,21 @@ AbsoluteConfigLocation(const char *location, const char 
*calling_file)

if (is_absolute_path(location))
return pstrdup(location);
+
+   if (calling_file != NULL)
+   {
+   strlcpy(abs_path, calling_file, sizeof(abs_path));
+   get_parent_directory(abs_path);
+   join_path_components(abs_path, abs_path, location);
+   }
else
{
-   if (calling_file != NULL)
-   {
-   strlcpy(abs_path, calling_file, sizeof(abs_path));
-   get_parent_directory(abs_path);
-   join_path_components(abs_path, abs_path, location);
-   canonicalize_path(abs_path);
-   }
-   else
-   {
-   AssertState(DataDir);
-   join_path_components(abs_path, DataDir, location);
-   canonicalize_path(abs_path);
-   }
-   return pstrdup(abs_path);
+   AssertState(DataDir);
+   join_path_components(abs_path, DataDir, location);
}
+
+   canonicalize_path(abs_path);
+   return pstrdup(abs_path);
 }

--
Best regards
Japin Li

Remove unnecessary else branch

[Li Japin]

Hi,

I found in guc-file.l we can omit the else branch in AbsoluteConfigLocation().

diff --git a/src/backend/utils/misc/guc-file.l 
b/src/backend/utils/misc/guc-file.l
index c98e220295..9d4b3d7236 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -522,23 +522,21 @@ AbsoluteConfigLocation(const char *location, const char 
*calling_file)

if (is_absolute_path(location))
return pstrdup(location);
+
+   if (calling_file != NULL)
+   {
+   strlcpy(abs_path, calling_file, sizeof(abs_path));
+   get_parent_directory(abs_path);
+   join_path_components(abs_path, abs_path, location);
+   canonicalize_path(abs_path);
+   }
else
{
-   if (calling_file != NULL)
-   {
-   strlcpy(abs_path, calling_file, sizeof(abs_path));
-   get_parent_directory(abs_path);
-   join_path_components(abs_path, abs_path, location);
-   canonicalize_path(abs_path);
-   }
-   else
-   {
-   AssertState(DataDir);
-   join_path_components(abs_path, DataDir, location);
-   canonicalize_path(abs_path);
-   }
-   return pstrdup(abs_path);
+   AssertState(DataDir);
+   join_path_components(abs_path, DataDir, location);
+   canonicalize_path(abs_path);
}
+   return pstrdup(abs_path);
 }


--
Best regards
Japin Li

Re: Optimize memory allocation code

[Li Japin]

On Sep 29, 2020, at 9:30 PM, Alvaro Herrera 
mailto:alvhe...@2ndquadrant.com>> wrote:

On 2020-Sep-26, Li Japin wrote:

Thanks! How big is this overhead? Is there any way I can test it？

You could also have a look at the assembly code that your compiler
generates -- particularly examine how it changes.

Thanks for your advice!

The origin assembly code for palloc0 is:

00517690 :
  517690: 55   push   %rbp
  517691: 53   push   %rbx
  517692: 48 89 fb mov%rdi,%rbx
  517695: 48 83 ec 08   sub$0x8,%rsp
  517699: 48 81 ff ff ff ff 3f cmp$0x3fff,%rdi
  5176a0: 48 8b 2d d9 0c 48 00 mov0x480cd9(%rip),%rbp# 998380 

  5176a7: 0f 87 d5 00 00 00 ja 517782 
  5176ad: 48 8b 45 10   mov0x10(%rbp),%rax
  5176b1: 48 89 fe mov%rdi,%rsi
  5176b4: c6 45 04 00   movb   $0x0,0x4(%rbp)
  5176b8: 48 89 ef mov%rbp,%rdi
  5176bb: ff 10 callq  *(%rax)
  5176bd: 48 85 c0 test   %rax,%rax
  5176c0: 48 89 c1 mov%rax,%rcx
  5176c3: 74 5b je 517720 
  5176c5: f6 c3 07 test   $0x7,%bl
  5176c8: 75 36 jne517700 
  5176ca: 48 81 fb 00 04 00 00 cmp$0x400,%rbx
  5176d1: 77 2d ja 517700 
  5176d3: 48 01 c3 add%rax,%rbx
  5176d6: 48 39 d8 cmp%rbx,%rax
  5176d9: 73 35 jae517710 
  5176db: 0f 1f 44 00 00   nopl   0x0(%rax,%rax,1)
  5176e0: 48 83 c0 08   add$0x8,%rax
  5176e4: 48 c7 40 f8 00 00 00 movq   $0x0,-0x8(%rax)
  5176eb: 00
  5176ec: 48 39 c3 cmp%rax,%rbx
  5176ef: 77 ef ja 5176e0 
  5176f1: 48 83 c4 08   add$0x8,%rsp
  5176f5: 48 89 c8 mov%rcx,%rax
  5176f8: 5b   pop%rbx
  5176f9: 5d   pop%rbp
  5176fa: c3   retq
  5176fb: 0f 1f 44 00 00   nopl   0x0(%rax,%rax,1)
  517700: 48 89 cf mov%rcx,%rdi
  517703: 48 89 da mov%rbx,%rdx
  517706: 31 f6 xor%esi,%esi
  517708: e8 e3 0e ba ff   callq  b85f0 
  51770d: 48 89 c1 mov%rax,%rcx
  517710: 48 83 c4 08   add$0x8,%rsp
  517714: 48 89 c8 mov%rcx,%rax
  517717: 5b   pop%rbx
  517718: 5d   pop%rbp
  517719: c3   retq
  51771a: 66 0f 1f 44 00 00 nopw   0x0(%rax,%rax,1)
  517720: 48 8b 3d 51 0c 48 00 mov0x480c51(%rip),%rdi# 998378 

  517727: be 64 00 00 00   mov$0x64,%esi
  51772c: e8 1f f9 ff ff   callq  517050 
  517731: 31 f6 xor%esi,%esi
  517733: bf 14 00 00 00   mov$0x14,%edi
  517738: e8 53 6d fd ff   callq  4ee490 
  51773d: bf c5 20 00 00   mov$0x20c5,%edi
  517742: e8 99 9b fd ff   callq  4f12e0 
  517747: 48 8d 3d 07 54 03 00 lea0x35407(%rip),%rdi# 54cb55 
<__func__.7554+0x45>
  51774e: 31 c0 xor%eax,%eax
  517750: e8 ab 9d fd ff   callq  4f1500 
  517755: 48 8b 55 38   mov0x38(%rbp),%rdx
  517759: 48 8d 3d 80 11 16 00 lea0x161180(%rip),%rdi# 6788e0 
<__func__.6248+0x150>
  517760: 48 89 de mov%rbx,%rsi
  517763: 31 c0 xor%eax,%eax
  517765: e8 56 a2 fd ff   callq  4f19c0 
  51776a: 48 8d 15 ff 11 16 00 lea0x1611ff(%rip),%rdx# 678970 
<__func__.7326>
  517771: 48 8d 3d 20 11 16 00 lea0x161120(%rip),%rdi# 678898 
<__func__.6248+0x108>
  517778: be eb 03 00 00   mov$0x3eb,%esi
  51777d: e8 0e 95 fd ff   callq  4f0c90 
  517782: 31 f6 xor%esi,%esi
  517784: bf 14 00 00 00   mov$0x14,%edi
  517789: e8 02 6d fd ff   callq  4ee490 
  51778e: 48 8d 3d db 10 16 00 lea0x1610db(%rip),%rdi# 678870 
<__func__.6248+0xe0>
  517795: 48 89 de mov%rbx,%rsi
  517798: 31 c0 xor%eax,%eax
  51779a: e8 91 98 fd ff   callq  4f1030 
  51779f: 48 8d 15 ca 11 16 00 lea0x1611ca(%rip),%rdx# 678970 
<__func__.7326>
  5177a6: 48 8d 3d eb 10 16 00 lea0x1610eb(%rip),%rdi# 678898 
<__func__.6248+0x108>
  5177ad: be df 03 00 00   mov$0x3df,%esi
  5177b2: e8 d9 94 fd ff   callq  4f0c90 
  5177b7: 66 0f 1f 84 00 00 00 nopw   0x0(%rax,%rax,1)
  5177be: 00 00

After modified, the palloc0 assembly code is:

00517690 :
  517690: 53   push   %rbx
  517691: 48 89 fb mov%rdi,%rbx
  517694: e8 17 ff ff ff   callq  5175b0 
  517699: f6 c3 07 test   $0x7,%bl
  51769c: 48 89 c1 mov%rax,%rcx
  51769f: 75 2f jne5176d0 
  5176a1: 48 81 fb 00 04 00 00 cmp$0x400,%rbx
  5176a8: 77 26 ja 5176d0 
  5176aa: 48 01 c3 add%rax,%rbx

Re: Optimize memory allocation code

[Li Japin]

> On Sep 26, 2020, at 8:09 AM, Julien Rouhaud  wrote:
> 
> Hi,
> 
> On Sat, Sep 26, 2020 at 12:14 AM Li Japin  wrote:
>> 
>> Hi, hackers!
>> 
>> I find the palloc0() is similar to the palloc(), we can use palloc() inside 
>> palloc0()
>> to allocate space, thereby I think we can reduce  duplication of code.
> 
> The code is duplicated on purpose.  There's a comment at the beginning
> that mentions it:
> 
>  /* duplicates MemoryContextAllocZero to avoid increased overhead */
> 
> Same for MemoryContextAllocZero() itself.

Thanks! How big is this overhead? Is there any way I can test it？

Best regards!

--
Japin Li

Optimize memory allocation code

[Li Japin]

Hi, hackers!

I find the palloc0() is similar to the palloc(), we can use palloc() inside 
palloc0()
to allocate space, thereby I think we can reduce  duplication of code.

Best regards!

--
Japin Li



0001-Optimize-memory-allocation-code.patch
Description: 0001-Optimize-memory-allocation-code.patch

Re: Parallelize stream replication process

[Li Japin]

Thanks for your advice!  This help me a lot.

> On Sep 17, 2020, at 9:18 PM, Jakub Wartak  wrote:
> 
> Li Japin wrote:
> 
>> If we can improve the efficiency of replay, then we can shorten the database 
>> recovery time (streaming replication or database crash recovery). 
> (..)
>> For streaming replication, we may need to improve the transmission of WAL 
>> logs to improve the entire recovery process.
>> I’m not sure if this is correct.
> 
> Hi, 
> 
> If you are interested in increased efficiency of WAL replay internals/startup 
> performance then you might be interested in following threads:
> 
> Cache relation sizes in recovery - 
> https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BNPZeEdLXAcNr%2Bw0YOZVb0Un0_MwTBpgmmVDh7No2jbg%40mail.gmail.com#feace7ccbb8e3df8b086d0a2217df91f
> Faster compactify_tuples() - 
> https://www.postgresql.org/message-id/flat/ca+hukgkmqfvpjr106grhwk6r-nxv0qoctrezuqzxgphesal...@mail.gmail.com
> Handing off SLRU fsyncs to the checkpointer - 
> https://www.postgresql.org/message-id/flat/CA%2BhUKGLJ%3D84YT%2BNvhkEEDAuUtVHMfQ9i-N7k_o50JmQ6Rpj_OQ%40mail.gmail.com
> Optimizing compactify_tuples() - 
> https://www.postgresql.org/message-id/flat/CA%2BhUKGKMQFVpjr106gRhwk6R-nXv0qOcTreZuQzxgpHESAL6dw%40mail.gmail.com
> Background bgwriter during crash recovery - 
> https://www.postgresql.org/message-id/flat/ca+hukgj8nrsqgkzensnrc2mfrobv-jcnacbyvtpptk2a9yy...@mail.gmail.com
> WIP: WAL prefetch (another approach) - 
> https://www.postgresql.org/message-id/flat/CA%2BhUKGJ4VJN8ttxScUFM8dOKX0BrBiboo5uz1cq%3DAovOddfHpA%40mail.gmail.com
> Division in dynahash.c due to HASH_FFACTOR - 
> https://www.postgresql.org/message-id/flat/VI1PR0701MB696044FC35013A96FECC7AC8F62D0%40VI1PR0701MB6960.eurprd07.prod.outlook.com
> [PATCH] guc-ify the formerly hard-coded MAX_SEND_SIZE to max_wal_send - 
> https://www.postgresql.org/message-id/flat/CACJqAM2uAUnEAy0j2RRJOSM1UHPdGxCr%3DU-HbqEf0aAcdhUoEQ%40mail.gmail.com
> Unnecessary delay in streaming replication due to replay lag - 
> https://www.postgresql.org/message-id/flat/CANXE4Tc3FNvZ_xAimempJWv_RH9pCvsZH7Yq93o1VuNLjUT-mQ%40mail.gmail.com
> WAL prefetching in future combined with AIO (IO_URING) - longer term future, 
> https://anarazel.de/talks/2020-05-28-pgcon-aio/2020-05-28-pgcon-aio.pdf
> 
> Good way to start is to profile the system what is taking time during Your 
> failover situation OR Your normal hot-standby behavior 
> and then proceed to identifying and characterizing the main bottleneck - 
> there can be many depending on the situation (inefficient single processes 
> PostgreSQL code, 
> CPU-bound startup/recovering, IOPS/VFS/ syscall/s / API limitations, single 
> TCP stream limitations  single TCP stream latency impact in WAN, contention 
> on locks in hot-standby case...) .
> 
> Some of the above are already commited in for 14/master, some are not and 
> require further discussions and testing. 
> Without real identification of the bottleneck and WAL stream statistics you 
> are facing , it's hard to say how would parallel WAL recovery improve the 
> situation.
> 
> -J.

Re: Parallelize stream replication process

[Li Japin]

> On Sep 15, 2020, at 3:41 PM, Fujii Masao  wrote:
> 
> 
> 
> On 2020/09/15 13:41, Bharath Rupireddy wrote:
>> On Tue, Sep 15, 2020 at 9:27 AM Li Japin  wrote:
>>> 
>>> For now, postgres use single process to send, receive and replay the WAL 
>>> when we use stream replication,
>>> is there any point to parallelize this process? If it does, how do we start?
>>> 
>>> Any thoughts?
> 
> Probably this is another parallelism than what you're thinking,
> but I was thinking to start up walwriter process in the standby server
> and make it fsync the streamed WAL data. This means that we leave
> a part of tasks of walreceiver process to walwriter. Walreceiver
> performs WAL receive and write, and walwriter performs WAL flush,
> in parallel. I'm just expecting that this change would improve
> the replication performance, e.g., reduce the time to wait for
> sync replication.
> 
> Without this change (i.e., originally), only walreceiver performs
> WAL receive, write and flush. So wihle walreceiver is fsyncing WAL data,
> it cannot receive newly-arrived WAL data. If WAL flush takes a time,
> which means that the time to wait for sync replication in the primary
> would be enlarged.
> 
> Regards,
> 
> -- 
> Fujii Masao
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION

Yeah, this might be a direction. 

Now I am thinking about how to parallel WAL log replay. If we can improve the 
efficiency
of replay, then we can shorten the database recovery time (streaming 
replication or database
crash recovery). 

For streaming replication, we may need to improve the transmission of WAL logs 
to improve
the entire recovery process.

I’m not sure if this is correct.

Regards,
Japin Li.

Re: Parallelize stream replication process

[Li Japin]

Thanks for clarifying the questions!

> On Sep 15, 2020, at 12:41 PM, Bharath Rupireddy 
>  wrote:
> 
> On Tue, Sep 15, 2020 at 9:27 AM Li Japin  wrote:
>> 
>> For now, postgres use single process to send, receive and replay the WAL 
>> when we use stream replication,
>> is there any point to parallelize this process? If it does, how do we start?
>> 
>> Any thoughts?
>> 
> 
> I think we must ask few questions:
> 
> 1. What's the major gain we get out of this? Is it that the time to
> stream gets reduced or something else?

I think when the database failover, we might shorten the recovery time from the 
parallel stream replication.

> If the answer to the above point is something solid, then
> 2. How do we distribute the work to multiple processes?
> 3. Do we need all of the workers to maintain the order in which they
> read WAL files(on the publisher) and apply the changes(on the
> subscriber?)
> 3. Do we want to map the sender/publisher workers to
> receiver/subscriber workers on a one-to-one basis? If not, how do we
> do it?
> 4. How do sender and receiver workers communicate?
> 5. What if we have multiple subscribers/receivers?
> 
> I'm no expert in replication, I may be wrong as well. Others may have
> better thoughts.
> 

Maybe we can distribute the work to multiple processes according by the WAL 
record type.

In the first step, I think we can parallel the replay process. We can classify 
the WAL by WAL type or RmgrId,
and then parallel those WAL replay if possible.

Then, we can think how to parallel WalReceiver and WalSender.

Best regards
Japin Li.

Parallelize stream replication process

[Li Japin]

Hi, hackers

For now, postgres use single process to send, receive and replay the WAL when 
we use stream replication,
is there any point to parallelize this process? If it does, how do we start?

Any thoughts?

Best regards

Japin Li

Re: Docs: inaccurate description about config settings

[Li Japin]

On Sep 1, 2020, at 8:20 PM, Ian Lawrence Barwick 
mailto:barw...@gmail.com>> wrote:

2020年9月1日(火) 19:37 Li Japin mailto:japi...@hotmail.com>>:

Hi, hackers

When I setup a stream replication I found that the documentation says that 
promote_trigger_file
parameter can only be set in the postgresql.conf file or on the server command 
line, however, it
can also be put into postgresql.auto.conf. If I use include to import a new 
config, it works too.

There are many parameters use this description:
$ grep 'This parameter can only be set in the' -rn doc/
(...)

I think this description is misleading. we should correct it, isn't it?

I must admit every time I see this wording, it strikes me as very specific
and potentially confusing given the alternative files the parameter could be
placed in.

I think it would be clearer for anyone not familiar with the configuration file
system to change occurrences of this wording to something like:

 This parameter can only be set in the configuration file

which would link to:

 
https://www.postgresql.org/docs/current/config-setting.html#CONFIG-SETTING-CONFIGURATION-FILE

which provides more information. Though on that page it seems like it would be
also sensible to bundle the section about include directives in the
configuration
file (19.1.5) together with the section about the configuration itself (19.1.2).

+1

Attached is a fix following Ian Barwick’s suggestion.

Regards,
Japin Li






fix-confusing-configuration-parameters-setting-desc.patch
Description: fix-confusing-configuration-parameters-setting-desc.patch

Docs: inaccurate description about config settings

[Li Japin]

Hi, hackers

When I setup a stream replication I found that the documentation says that 
promote_trigger_file
parameter can only be set in the postgresql.conf file or on the server command 
line, however, it
can also be put into postgresql.auto.conf. If I use include to import a new 
config, it works too.

There are many parameters use this description:
$ grep 'This parameter can only be set in the' -rn doc/
doc/src/sgml/sepgsql.sgml:273:  This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1001:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1040:for details. This parameter can only be 
set in the
doc/src/sgml/config.sgml:1071:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1133:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1151:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1169:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1187:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1204:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1232:This parameter can only be set in the
doc/src/sgml/config.sgml:1307:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1334:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1411:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1445:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:1471:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:2180: of 10.  This parameter can only be set 
in the
doc/src/sgml/config.sgml:2199: This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:2227: This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:2259: This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:2817:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:2863:This parameter can only be set in the 
postgresql.conf
doc/src/sgml/config.sgml:3012:next higher multiple of 10. This 
parameter can only be set in the
doc/src/sgml/config.sgml:3036:This parameter can only be set in the
….

I think this description is misleading. we should correct it, isn't it?

Regards,
Japin Li

Re: Terminate the idle sessions

[Li Japin]

> On Aug 31, 2020, at 11:43 AM, Thomas Munro  wrote:
> 
> On Mon, Aug 31, 2020 at 2:40 PM Li Japin  wrote:
>> Could you give the more details about the test instructions?
> 
> Hi Japin,
> 
> Sure.  Because I wasn't trying to get reliable TPS number or anything,
> I just used a simple short read-only test with one connection, like
> this:
> 
> pgbench -i -s10 postgres
> pgbench -T60 -Mprepared -S postgres
> 
> Then I looked for the active backend and ran strace -c -p XXX for a
> few seconds and hit ^C to get the counters.  I doubt the times are
> very accurate, but the number of calls is informative.
> 
> If you do that on a server running with -c statement_timeout=10s, you
> see one setitimer() per transaction.  If you also use -c
> idle_session_timeout=10s at the same time, you see two.

Hi, Thomas,

Thanks for your point out this problem, here is the comparison.

Without Optimize settimer usage:
% time seconds  usecs/call callserrors syscall
-- --- --- - - 
 41.221.444851   1   1317033   setitimer
 28.410.995936   2658622   sendto
 24.630.863316   1659116   599 recvfrom
  5.710.200275   2111055   pread64
  0.030.001152   2   599   epoll_wait
  0.000.00   0 1   epoll_ctl
-- --- --- - - 
100.003.505530   2746426   599 total

With Optimize settimer usage:
% time seconds  usecs/call callserrors syscall
-- --- --- - - 
 49.891.464332   1   1091429   sendto
 40.831.198389   1   1091539   219 recvfrom
  9.260.271890   1183321   pread64
  0.020.000482   2   214   epoll_wait
  0.000.13   3 5   setitimer
  0.000.10   2 5   rt_sigreturn
  0.000.00   0 1   epoll_ctl
-- --- --- - - 
100.002.935116   2366514   219 total

Here’s a modified version of Thomas’s patch.



v3-0001-Allow-terminating-the-idle-sessions.patch
Description: v3-0001-Allow-terminating-the-idle-sessions.patch


v3-0002-Optimize-setitimer-usage.patch
Description: v3-0002-Optimize-setitimer-usage.patch

Re: Terminate the idle sessions

[Li Japin]

On Aug 31, 2020, at 8:51 AM, Thomas Munro 
mailto:thomas.mu...@gmail.com>> wrote:

The main problem I have with it is the high frequency setitimer()
calls.  If you enable both statement_timeout and idle_session_timeout,
then we get up to huge number of system calls, like the following
strace -c output for a few seconds of one backend under pgbench -S
workload shows:

% time seconds  usecs/call callserrors syscall
-- --- --- - - 
39.450.118685   0250523   setitimer
29.980.090200   0125275   sendto
24.300.073107   0126235   973 recvfrom
 6.010.018068   0 20950   pread64
 0.260.000779   0   973   epoll_wait
-- --- --- - - 
100.000.300839523956   973 total

Hi, Thomas,

Could you give the more details about the test instructions?

Re: file_fdw vs relative paths

[Li Japin]

On Aug 25, 2020, at 8:26 AM, Bruce Momjian 
mailto:br...@momjian.us>> wrote:

Yes, I tested back to 9.5 too:

CREATE EXTENSION file_fdw;
CREATE SERVER pgconf FOREIGN DATA WRAPPER file_fdw;
CREATE FOREIGN TABLE pgconf (line TEXT) SERVER pgconf OPTIONS ( filename
'postgresql.conf', format 'text', delimiter E'\x7f' );
SELECT * FROM pgconf;
 # -
 # PostgreSQL configuration file
 # -
 #
 # This file consists of lines of the form:
…

The file_fdw extension was introduced by commit 
7c5d0ae7078456bfeedb2103c45b9a32285c2631,
and I tested it supports relative paths.  This is a doc bug.

--
Japin Li

Re: Terminate the idle sessions

[Li Japin]

On Aug 18, 2020, at 9:19 AM, Kyotaro Horiguchi 
mailto:horikyota@gmail.com>> wrote:

The same already happens for idle_in_transaction_session_timeout and
we can use "ALTER ROLE/DATABASE SET" to dislable or loosen them, it's
a bit cumbersome, though. I don't think we should (at least
implicitly) disable those timeouts ad-hockerly for postgres_fdw.

+1.

Re: Terminate the idle sessions

[Li Japin]

On Aug 14, 2020, at 2:15 PM, Bharath Rupireddy 
mailto:bharath.rupireddyforpostg...@gmail.com>>
 wrote:

I think, since the idle_session_timeout is by default disabled, we
have no problem. My thought is what if a user enables the
feature(knowingly or unknowingly) on the remote backend? If the user
knows about the above scenario, that may be fine. On the other hand,
either we can always the feature on the remote backend(at the
beginning of the remote txn, like we set for some other configuration
settings see - configure_remote_session() in connection.c) or how
about mentioning the above scenario in this feature documentation?

Though we can disable the idle_session_timeout when using postgres_fdw,
there still has locally cached connection cache entries when the remote sessions
terminated by accident.  AFAIK, you have provided a patch to solve this
problem, and it is in current CF [1].

[1] - https://commitfest.postgresql.org/29/2651/

Best Regards,
Japin Li.

Re: Terminate the idle sessions

[Li Japin]

Hi,

On Aug 11, 2020, at 5:42 AM, Cary Huang 
mailto:cary.hu...@highgo.ca>> wrote:

I applied this patch to the PG13 branch and generally this feature works as 
described. The new "idle_session_timeout" that controls the idle session 
disconnection is not in the default postgresql.conf and I think it should be 
included there with default value 0, which means disabled.

Thanks for looking at it!

I’ve attached a new version that add “idle_session_timeout” in the default 
postgresql.conf.

On Mon, Aug 10, 2020 at 2:43 PM Cary Huang 
mailto:cary.hu...@highgo.ca>> wrote:
There is currently no enforced minimum value for "idle_session_timeout" (except 
for value 0 for disabling the feature), so user can put any value larger than 0 
and it could be very small like 500 or even 50 millisecond, this would make any 
psql connection to disconnect shortly after it has connected, which may not be 
ideal. Many systems I have worked with have 30 minutes inactivity timeout by 
default, and I think it would be better and safer to enforce a reasonable 
minimum timeout value

I'd accept a value of say 1,000 being minimum in order to reinforce the fact 
that a unit-less input, while possible, is taken to be milliseconds and such 
small values most likely mean the user has made a mistake.  I would not choose 
a minimum allowed value solely based on our concept of "reasonable".  I don't 
imagine a value of say 10 seconds, while seemingly unreasonable, is going to be 
unsafe.

I think David is right, see “idle_in_transaction_session_timeout”, it also 
doesn’t have a “reasonable” minimum value.



v2-0001-Allow-terminating-the-idle-sessions.patch
Description: v2-0001-Allow-terminating-the-idle-sessions.patch

Re: Terminate the idle sessions

[Li Japin]

On Jun 10, 2020, at 10:27 PM, Adam Brusselback 
mailto:adambrusselb...@gmail.com>> wrote:

My use case is, I have a primary application that connects to the DB, most 
users work through that (setting is useless for this scenario, app manages it's 
connections well enough). I also have a number of internal users who deal with 
data ingestion and connect to the DB directly to work, and those users 
sometimes leave query windows open for days accidentally. Generally not an 
issue, but would be nice to be able to time those connections out.

If there is no big impact, I think we might add it builtin.

Japin Li

Re: Terminate the idle sessions

[Li Japin]

On Jun 10, 2020, at 4:25 PM, Michael Paquier 
mailto:mich...@paquier.xyz>> wrote:

Idle sessions staying around can be a problem in the long run as they
impact snapshot building.  You could for example use a background
worker to do this work, like that:
https://github.com/michaelpq/pg_plugins/tree/master/kill_idle

Why not implement it in the core of Postgres? Are there any disadvantages of
implementing it in the core of Postgres?

Japin Li

Re: Terminate the idle sessions

[Li Japin]

On Jun 9, 2020, at 10:35 PM, David G. Johnston 
mailto:david.g.johns...@gmail.com>> wrote:

I’m curious as to the use case because I cannot imagine using this.  Idle 
connections are normal.  Seems better to monitor them and conditionally execute 
the disconnect backend function from the monitoring layer than indiscriminately 
disconnect based upon time.

I agree with you.  But we can also give the user to control the idle sessions 
lifetime.

Terminate the idle sessions

[Li Japin]

Hi, hackers

When some clients connect to database in idle state, postgres do not close the 
idle sessions,
here i add a new GUC idle_session_timeout to let postgres close the idle 
sessions, it samilar
to idle_in_transaction_session_timeout.

Best, regards.

Japin Li



0001-Allow-terminating-the-idle-sessions.patch
Description: 0001-Allow-terminating-the-idle-sessions.patch

Code cleanup for build_regexp_split_result

[Li Japin]

Hi hackers,

I find the build_regexp_split_result() has redundant codes, we can move it to 
before the condition check, can we?

Best regards.

Japin Li






0001-Code-cleanup-for-build_regexp_split_result.patch
Description: 0001-Code-cleanup-for-build_regexp_split_result.patch

Re: Duplicate function call on timestamp2tm

[Li Japin]

Thanks for your confirm. Is there anything I can do?

On Dec 12, 2019, at 11:13 PM, Tom Lane 
mailto:t...@sss.pgh.pa.us>> wrote:

Ah, after looking in the git history, not quite that ancient:
this duplication dates to commit 258ee1b63, which moved these
switch cases from the "if (type == RESERV)" switches in the
same functions.  In the previous coding these function calls
were actually necessary, but here they're redundant.  I guess
that's just additional ammunition for Greg's point that the
keywords were misclassified ;-).

Duplicate function call on timestamp2tm

[Li Japin]

Hi,

I find there is a duplicate function call on timestamp2tm in timestamptz_part 
and timestamp_part.
Is that necessary? I remove the latter one and it also works.

Best,
Japin.



remove-duplicate-timestamp2tm-function-call.patch
Description: remove-duplicate-timestamp2tm-function-call.patch

Re: Built-in connection pooler

[Li Japin]

Hi, Konstantin

I test the patch-16 on postgresql master branch, and I find the 
temporary table
cannot removed when we re-connect to it. Here is my test:

japin@ww-it:~/WwIT/postgresql/Debug/connpool$ initdb
The files belonging to this database system will be owned by user "japin".
This user must also own the server process.

The database cluster will be initialized with locale "en_US.UTF-8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".

Data page checksums are disabled.

creating directory /home/japin/WwIT/postgresql/Debug/connpool/DATA ... ok
creating subdirectories ... ok
selecting dynamic shared memory implementation ... posix
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting default time zone ... Asia/Shanghai
creating configuration files ... ok
running bootstrap script ... ok
performing post-bootstrap initialization ... ok
syncing data to disk ... ok

initdb: warning: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the option -A, or
--auth-local and --auth-host, the next time you run initdb.

Success. You can now start the database server using:

     pg_ctl -D /home/japin/WwIT/postgresql/Debug/connpool/DATA -l 
logfile start

japin@ww-it:~/WwIT/postgresql/Debug/connpool$ pg_ctl -l /tmp/log start
waiting for server to start done
server started
japin@ww-it:~/WwIT/postgresql/Debug/connpool$ psql postgres
psql (13devel)
Type "help" for help.

postgres=# ALTER SYSTEM SET connection_proxies TO 1;
ALTER SYSTEM
postgres=# ALTER SYSTEM SET session_pool_size TO 1;
ALTER SYSTEM
postgres=# \q
japin@ww-it:~/WwIT/postgresql/Debug/connpool$ pg_ctl -l /tmp/log restart
waiting for server to shut down done
server stopped
waiting for server to start done
server started
japin@ww-it:~/WwIT/postgresql/Debug/connpool$ psql -p 6543 postgres
psql (13devel)
Type "help" for help.

postgres=# CREATE TEMP TABLE test(id int, info text);
CREATE TABLE
postgres=# INSERT INTO test SELECT id, md5(id::text) FROM 
generate_series(1, 10) id;
INSERT 0 10
postgres=# select * from pg_pooler_state();
  pid  | n_clients | n_ssl_clients | n_pools | n_backends | 
n_dedicated_backends | n_idle_backends | n_idle_clients | tx_bytes | 
rx_bytes | n_transactions
--+---+---+-++--+-++--+--+
  3885 | 1 | 0 |   1 |  1 
|    0 |   0 |  0 | 1154 | 
2880 |  6
(1 row)

postgres=# \q
japin@ww-it:~/WwIT/postgresql/Debug/connpool$ psql -p 6543 postgres
psql (13devel)
Type "help" for help.

postgres=# \d
     List of relations
   Schema   | Name | Type  | Owner
---+--+---+---
  pg_temp_3 | test | table | japin
(1 row)

postgres=# select * from pg_pooler_state();
  pid  | n_clients | n_ssl_clients | n_pools | n_backends | 
n_dedicated_backends | n_idle_backends | n_idle_clients | tx_bytes | 
rx_bytes | n_transactions
--+---+---+-++--+-++--+--+
  3885 | 1 | 0 |   1 |  1 
|    0 |   0 |  0 | 2088 | 
3621 |  8
(1 row)

postgres=# select * from test ;
  id |   info
+--
   1 | c4ca4238a0b923820dcc509a6f75849b
   2 | c81e728d9d4c2f636f067f89cc14862c
   3 | eccbc87e4b5ce2fe28308fd9f2a7baf3
   4 | a87ff679a2f3e71d9181a67b7542122c
   5 | e4da3b7fbbce2345d7772b0674a318d5
   6 | 1679091c5a880faf6fb5e6087eb1b2dc
   7 | 8f14e45fceea167a5a36dedd4bea2543
   8 | c9f0f895fb98ab9159f51fd0297e236d
   9 | 45c48cce2e2d7fbdea1afc51c7c6ad26
  10 | d3d9446802a44259755d38e6d163e820
(10 rows)

I inspect the code, and find the following code in DefineRelation function:

if (stmt->relation->relpersistence != RELPERSISTENCE_TEMP
     && stmt->oncommit != ONCOMMIT_DROP)
     MyProc->is_tainted = true;

For temporary table, MyProc->is_tainted might be true, I changed it as 
following:

if (stmt->relation->relpersistence == RELPERSISTENCE_TEMP
     || stmt->oncommit == ONCOMMIT_DROP)
     MyProc->is_tainted = true;

For temporary table, it works. I not sure the changes is right.

On 8/2/19 7:05 PM, Konstantin Knizhnik wrote:
>
>
> On 02.08.2019 12:57, DEV_OPS wrote:
>> Hello Konstantin
>>
>>
>> would you please re-base this patch? I'm going to test it, and back port
>> into PG10 stable and PG9 stable
>>
>>
>> thank you very much
>>
>>
>
> Thank you.
> Rebased patch is attached.
>
>

Re: Symbol referencing errors

[Li Japin]

Hi,

Finally, I find this crash is caused by shmget_osm, which does not support 
SmartOS (maybe,
I am not sure). When I install Oracle Instant Client 12.2.0.1.0, it works.

https://github.com/laurenz/oracle_fdw/issues/313


On 4/23/19 3:09 PM, Laurenz Albe wrote:

On Tue, 2019-04-23 at 04:26 +, Li Japin wrote:


Yes, those errors does not impact the postgresql, but when
I use oracle_fdw extension, I couldn't startup the postgresql,
and I find that the dlopen throw an error which lead postmaster
exit, and there is not more information.


That may wall be a bug in oracle_fdw, since I have no reports of
anybody running it on that operating system.

Maybe you should open an oracle_fdw issue, but I don't know how
much I can help you, since this is the first time I have heard
of SmartOS.

Re: Symbol referencing errors

[Li Japin]

On 4/23/19 12:09 PM, Tom Lane wrote:
> AFAICT they're harmless, so my advice is just ignore them.
>
> If you're sufficiently annoyed by them to find the cause
> and try to fix it, go ahead, but I haven't heard anyone
> else worried about it.  It might be that SmartOS wants
> something like what we have to do on macOS and AIX,
> ie provide the core postgres executable in some sort of
> linker switch while linking shlibs that will be loaded
> by that executable.
Yes, those errors does not impact the postgresql, but when
I use oracle_fdw extension, I couldn't startup the postgresql,
and I find that the dlopen throw an error which lead postmaster
exit, and there is not more information.


regards,

Japin Li

Symbol referencing errors

[Li Japin]

Hi,

When I compile PostgreSQL-11.2 on SmartOS, I find the following errors:

Undefined            first referenced
  symbol                  in file
per_MultiFuncCall   adminpack.o
end_MultiFuncCall   adminpack.o
BuildTupleFromCStrings  adminpack.o
DecodeDateTime  adminpack.o
TupleDescGetAttInMetadata   adminpack.o
path_is_prefix_of_path  adminpack.o
canonicalize_path   adminpack.o
text_to_cstring adminpack.o
errmsg  adminpack.o
superuser   adminpack.o
errcode_for_file_access adminpack.o
palloc  adminpack.o
CurrentMemoryContext    adminpack.o
pstrdup adminpack.o
ReadDir adminpack.o
FreeFile    adminpack.o
errfinish   adminpack.o
init_MultiFuncCall  adminpack.o
errstart    adminpack.o
AllocateDir adminpack.o
GetUserId   adminpack.o
is_member_of_role   adminpack.o
psprintf    adminpack.o
DataDir adminpack.o
Log_filename    adminpack.o
Log_directory   adminpack.o
AllocateFile    adminpack.o
path_is_relative_and_below_cwd  adminpack.o
HeapTupleHeaderGetDatum adminpack.o
errcode adminpack.o
FreeDir adminpack.o
ParseDateTime   adminpack.o
path_contains_parent_reference  adminpack.o
pg_detoast_datum_packed adminpack.o
CreateTemplateTupleDesc adminpack.o
TupleDescInitEntry  adminpack.o
ld: warning: symbol referencing errors
make[1]: Leaving directory 
'/home/postgres/postgresql-11.2/contrib/adminpack'


My environment is:

# cat /etc/release
     SmartOS x86_64
   Copyright 2010 Sun Microsystems, Inc.  All Rights Reserved.
    Copyright 2015 Joyent, Inc.  All Rights Reserved.
     Use is subject to license terms.
    See joyent_20161108T160947Z for assembly date and time.
# $ pg_config
BINDIR = /home/postgres/pg11.2/bin
DOCDIR = /home/postgres/pg11.2/share/doc
HTMLDIR = /home/postgres/pg11.2/share/doc
INCLUDEDIR = /home/postgres/pg11.2/include
PKGINCLUDEDIR = /home/postgres/pg11.2/include
INCLUDEDIR-SERVER = /home/postgres/pg11.2/include/server
LIBDIR = /home/postgres/pg11.2/lib
PKGLIBDIR = /home/postgres/pg11.2/lib
LOCALEDIR = /home/postgres/pg11.2/share/locale
MANDIR = /home/postgres/pg11.2/share/man
SHAREDIR = /home/postgres/pg11.2/share
SYSCONFDIR = /home/postgres/pg11.2/etc
PGXS = /home/postgres/pg11.2/lib/pgxs/src/makefiles/pgxs.mk
CONFIGURE = '--prefix=/home/postgres/pg11.2' 'CFLAGS=-g -O0'
CC = gcc
CPPFLAGS =
CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute 
-Wformat-security -fno-strict-aliasing -fwrapv 
-fexcess-precision=standard -g -O0
CFLAGS_SL = -fPIC
LDFLAGS = -Wl,-R'/home/postgres/pg11.2/lib'
LDFLAGS_EX =
LDFLAGS_SL =
LIBS = -lpgcommon -lpgport -lz -lreadline -lnsl -lsocket -lm
VERSION = PostgreSQL 11.2

Can anyone help me out? Thanks!

Best regards!

Japin Li