Dear Wang,
Thank you for making a patch.
I applied your patch and confirmed that codes passed regression test.
I put a short reviewing:
```
+ static int skipped_changes_count = 0;
+ /*
+* Conservatively, at least 150,000 changes can be skipped in 1s.
+*
+* Beca
Dear Hackers,
I found patches we depend have been committed, so rebased.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=50e570a59e7f86bb41f029a66b781fc79b8d50f1
In this version there is a little bit change in part of postgres_fdw.
A system checking by WaitEventSetCanReportClosed()
Dear Horiguchi-san,
Thank you for giving your suggestions. I want to confirm your saying.
> FWIW, I'm not sure this feature necessarily requires core support
> dedicated to FDWs. The core have USER_TIMEOUT feature already and
> FDWs are not necessarily connection based. It seems better if FDWs
> I understood here as removing following mechanism from core:
>
> * disable timeout at end of tx.
While reading again and this part might be wrong.
Sorry for inconvenience.
But anyway some codes should be (re)moved from core, right?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Dear Horiguchi-san,
> I think we just don't need to add the special timeout kind to the
> core. postgres_fdw can use USER_TIMEOUT and it would be suffiction to
> keep running health checking regardless of transaction state then fire
> query cancel if disconnection happens. As I said in the previo
Hi hackers,
While reviewing [1], I and Amit noticed that a flag ParallelMessagePending is
defined
as "volatile bool", but other flags set by signal handlers are defined as
"volatile sig_atomic_t".
The datatype has been defined in standard C,
and it says that variables referred by signal handler
Dear Wang,
Thanks for updating patch!... but cfbot says that it cannot be accepted [1].
I thought the header should be included, like miscadmin.h.
[1]: https://cirrus-ci.com/task/5909508684775424
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Dear Michael,
> Done this one. I have scanned the code, but did not notice a similar
> mistake.
I found your commit, thanks!
> It is worth noting that we have only one remaining "volatile
> bool" in the headers now.
Maybe you mentioned about sigint_interrupt_enabled,
and it also seems to be m
Dear Wang,
Followings are comments for your patchset.
0001
01. launcher.c - logicalrep_worker_stop_internal()
```
+
+ Assert(LWLockHeldByMe(LogicalRepWorkerLock));
+
```
I think it should be Assert(LWLockHeldByMeInMode(LogicalRepWorkerLock,
LW_SHARED))
because the lock is released
Dear Michael,
> Yeah, at least as of the cancel callback psql_cancel_callback() that
> handle_sigint() would call on SIGINT as this is set by psql. So it
> does not seem right to use a boolean rather than a sig_atomic_t in
> this case, as well.
PSA fix patch. Note that PromptInterruptContext.ena
Dear Önder:
Thank you for updating patch!
Your documentation seems OK, and I could not find any other places to be added
Followings are my comments.
01 relation.c - general
Many files are newly included.
I was not sure but some codes related with planner may be able to move to
src/backen
Hi hackers,
While running `make check LANC=C` with 32-bit virtual machine,
I found that it was failed at "aggregates". PSA the a1b3bca1_regression.diffs.
IIUC that part has been added by db0d67db.
I checked out the source, tested, and got same result. PSA the
db0d67db_regression.diffs
I'm not s
Dear Hou,
Thanks for updating patch. I will review yours soon, but I reply to your
comment.
> > 04. applyparallelworker.c - LogicalParallelApplyLoop()
> >
> > ```
> > + shmq_res = shm_mq_receive(mqh, &len, &data, false);
> > ...
> > + if (ConfigReloadPending)
> > +
Dear Tom,
> Hmm, we're not seeing any such failures in the buildfarm's 32-bit
> animals, so there must be some additional condition needed to make
> it happen. Can you be more specific?
Hmm, I was not sure about additional conditions, sorry.
I could reproduce with followings steps:
$ git clone
Dear Andres,
> This seems to reliably fail on windows. See
Thanks for reporting. Actually this feature cannot be used on Windows machine.
To check the status of each socket that connects to the foreign server,
the socket event WL_SOCKET_CLOSED is used.
The event is only enabled on some OSes, and
Dear Önder,
Thank you for being interest to my patch! Your suggestions will be included to
newer version.
> In other words, what is the trade-off for calling
> pgfdw_connection_check_internal() inside GetConnection() when we are about
> to use a "cached" connection? I think that might simplify t
Dear Önder,
> As far as I can see this patch is mostly useful for detecting the failures
> on the initial remote command. This is especially common when the remote
> server does a failover/switchover and postgres_fdw uses a cached connection
> to access to the remote server.
Sounds reasonable. Do
Dear Önder,
Thank you for updating the patch! At first I replied to your comments.
> My thinking on those functions is that they should probably stay
> in src/backend/replication/logical/relation.c. My main motivation is that
> those functions are so much tailored to the purposes of this file tha
Dear Amit,
> Can't we use WaitLatch in the case of SHM_MQ_WOULD_BLOCK as we are
> using it for the same case at some other place in the code? We can use
> the same nap time as we are using in the leader apply worker.
I'm not sure whether such a short nap time is needed or not.
Because unlike lead
Dear Hou,
I put comments for v35-0001.
01. catalog.sgml
```
+ Controls how to handle the streaming of in-progress transactions:
+ f = disallow streaming of in-progress transactions,
+ t = spill the changes of in-progress transactions to
+ disk and apply at once after the
Dear Hou,
> Thanks for the suggestion.
>
> I tried to add a WaitLatch, but it seems affect the performance
> because the Latch might not be set when leader send some
> message to parallel apply worker which means it will wait until
> timeout.
Yes, currently it leader does not notify anything.
To
Dear Önder,
Thanks for updating the patch! I checked yours and almost good.
Followings are just cosmetic comments.
===
01. relation.c - GetCheapestReplicaIdentityFullPath
```
* The reason that the planner would not pick partial indexes and
indexes
* with only expressions based
Dear Önder,
Thank you for updating the patch!
> It is not about CREATE INDEX being async. It is about pg_stat_all_indexes
> being async. If we do not wait, the tests become flaky, because sometimes
> the update has not been reflected in the view immediately.
Make sense, I forgot how stats collec
Dear Sawada-san,
Thank you for reviewing HEAD patch! PSA v3 patch.
> +# Test that we can force the top transaction to do timetravel when one of sub
> +# transactions needs that. This is necessary when we restart decoding
> from RUNNING_XACT
> +# without the wal to associate subtransaction to its
Dear Sawada-san,
> FYI, as I just replied to the related thread[1], the assertion failure
> in REL14 and REL15 can be fixed by the patch proposed there. So I'd
> like to see how the discussion goes. Regardless of this proposed fix,
> the patch proposed by Kuroda-san is required for HEAD, REL14, an
Dear Önder,
Thanks for updating the patch!
I think your saying seems reasonable.
I have no comments anymore now. Thanks for updating so quickly.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Hi hackers,
I found that the test catalog_change_snapshot was missed in
test_decoding/meson.build file.
PSA the patch to fix it.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
0001-add-missing-test-to-test_decoding-meson.build.patch
Description: 0001-add-missing-test-to-test_decoding-meson.build
Dear Michael,
> Thanks, applied. This was an oversight of 7f13ac8, and the CI accepts
> the test.
I confirmed your commit. Great thanks!
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Dear Önder,
Thanks for giving suggestions!
> Still, the reestablish mechanism can be further simplified with
> WL_SOCKET_CLOSED event such as the following (where we should probably
> rename pgfdw_connection_check_internal):
Sounds reasonable.
I think it may be included in this patch. I will try
Dear Horiguchi-san,
> Might be on slight different direction, but it looks to me a bit too
> much to use WaitEventSet to check only if a socket is live or not.
>
> A quick search in the tree told me that we could use pqSocketCheck()
> instead, and I think it would be the something that "could pot
Dear Osumi-san,
> I mainly followed the steps there and
> replaced the command "SELECT" for the remote table at 6-9 with "INSERT"
> command.
> Then, after waiting for few seconds, the "COMMIT" succeeded like below output,
> even after the server stop of the worker side.
> Additionally, the last r
Dear Sawada-san, Amit,
> IIUC Change-2 is required in v16 and HEAD but not mandatory in v15 and
> v14. The reason why we need Change-2 is that there is a case where we
> mark only subtransactions as containing catalog change while not doing
> that for its top-level transaction. In v15 and v14, sin
Dear Hou,
Thanks for updating the patch! Followings are my comments.
===
01. applyparallelworker.c - SIZE_STATS_MESSAGE
```
/*
* There are three fields in each message received by the parallel apply
* worker: start_lsn, end_lsn and send_time. Because we have updated these
* statistics in the
Hi Hackers,
I played pgbench with wrong parameters, and I found bug-candidate.
The latest commit in my source is 3a09d75.
1. Do initdb and start.
2. Initialize schema and data with "scale factor" = 1.
3. execute following command many times:
$ pgbench -c 101 -j 10 postgres
Then, sometimes the n
Dear Fabien,
Thank you for replying!
> Hmmm. Possibly. Another option could be not to report anything after some
> errors. I'm not sure, because it would depend on the use case. I guess the
> command returned an error status as well.
I did not know any use cases and decisions , but I vote to r
Dear Michael,
Thank you for replying!
> it does not strike me as a great idea to have a duplicate
> logic doing the parsing of URIs, even if libpq accepts multiple
> hosts/ports as an option.
Yeah, I agree your argument that duplicated parse function should be removed.
ECPG parses connection st
Dear Hackers,
I checked about DECLARE STATEMENT(added from ad8305a), and I noticed that
this connection-control feature cannot be used for DEALLOCATE and DESCRIBE
statement.
I attached the patch that fixes these bugs, this contains source and test code.
How do you think?
Best Regards,
Hayato
Dear Horiguchi-san,
Thank you for replying!
> (Maybe by consulting the code.. Anyway, )
I noticed the patch cannot be applied...
> The follwoing commands don't.
> DESCRIBE
> DEALLOCATE
> DECLARE CURSOR .. FOR
> CREATE TABLE AS EXECUTE
I'm not sure about `CREATE TABLE AS EXECUTE`(I'll check you
Dear Hackers,
I revised my patch.
> Please also ensure that you're generating the patch against git HEAD.
> The cfbot shows it as failing to apply, likely because you're looking
> at something predating ad8305a43d1890768a613d3fb586b44f17360f29.
Maybe there was something wrong with my local envir
Dear Michael,
> I have been chewing on this comment and it took me some time to
> understand what you meant here.
Sorry... But your understanding is correct.
> It is true that the ecpglib part, aka
> all the routines you are quoting above, don't rely at all on the
> connection names. However, t
Dear Horiguchi-san,
Thank you for reviewing! I attached new version.
Sorry for delaying reply.
> Since we don't allow descriptors with the same name even if they are
> for the different connections, I think we can set the current
> connection if any (which is set either by AT option or statement-
Hi hackers,
I created a small PoC. Please see the attached patches.
REQUIREMENT
Before patching them, patches in [1] must also be applied.
DIFFERENCES FROM PREVIOUS DESCRIPTIONS
* LRG is now implemented as SQL functions, not as a contrib module.
* New tables are added as system catalogs. Ther
Dear hackers,
I found another use-case for LRG. It might be helpful for migration.
LRG for migration
--
LRG may be helpful for machine migration, OS upgrade,
or PostgreSQL itself upgrade.
Assumes that users want to migrate database to other environment,
e
Dear Takahashi-san,
Thanks for giving feedbacks!
> > I don't know if it requires the kind of code you are thinking but I
> > agree that it is worth considering implementing it as an extension.
>
> I think the other advantage to implement as an extension is that users could
> install the extensio
Hi,
> > I noticed that we didn't collect the ObjectAddress returned by
> > ATExec[Attach|Detach]Partition. I think collecting this information can
> > make it
> > easier for users to get the partition OID of the attached or detached table
> > in
> > the event trigger. So how about collecting it
Dear Jelte,
I like your idea. But do we have to sort randomly even if target_session_attr
is set to 'primary' or 'read-write'?
I think this parameter can be used when all listed servers have same data,
and we can assume that one of members is a primary and others are secondary.
In this case use
Dear Hou-san,
> Thanks for having a look. It was a bit difficult to add a test for this.
> Because we currently don't have a user function which can return these
> collected ObjectAddresses for ALTER TABLE. And It seems we don't have tests
> for
> already collected ObjectAddresses as well :(
> Th
Dear Wang-san,
Hi, I'm also interested in the patch and I started to review this.
Followings are comments about 0001.
1. terminology
In your patch a new worker "apply background worker" has been introduced,
but I thought it might be confused because PostgreSQL has already the worker
"background
Dear Jelte,
> With plain Postgres this assumption is probably correct. But the main reason
> I'm interested in this patch was because I would like to be able to load
> balance across the workers in a Citus cluster. These workers are all
> primaries.
> Similar usage would likely be possible with B
Dear Wang,
I found further comments about the test code.
11. src/test/regress/sql/subscription.sql
```
-- fail - streaming must be boolean
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false, streaming = foo);
```
The comment i
Dear Wang,
Thanks for updating patch sets! Followings are comments about v20-0001.
1. config.sgml
```
Specifies maximum number of logical replication workers. This includes
both apply workers and table synchronization workers.
```
I think you can add a descriptio
Hi Wang,
> 6.a
>
> It seems that the upper line represents the apply background worker, but I
> think
> last_msg_send_time and last_msg_receipt_time should be null.
> Is it like initialization mistake?
I checked again about the issue.
Attributes worker->last_send_time, worker->last_recv_time,
Dear Fujii-san,
Thank you for replying! I attached new version.
> Why did you make even %u and %d available in application_name?
Actually no particular reason. I added them because they can easily add...
And I agree what you say, so removed.
> So some people may want to specify their own ID in
Dear Fujii-san,
I attached new version, that almost all codes
moved from libpq to postgres_fdw.
Now we can accept four types of escapes.
All escapes will be rewritten to connection souce's information:
* application_name,
* user name,
* database name, and
* backend's pid.
These are cannot be se
Dear Fujii-san,
> Can we split the patch into two as follows? If so, we can review
> and commit them one by one.
>
> #1. Add application_name GUC into postgres_fdw
> #2. Allow to specify special escape characters in application_name that
> postgres_fdw uses.
OK, I split and attached like that. 0
Dear Fujii-san,
Thank you for reviewing!
> This GUC parameter should be set after the options of foreign server
> are set so that its setting can override the server-level ones.
> Isn't it better to comment this?
I added the following comments:
```diff
- /* Use "postgres_fdw" as f
Dear Fujii-san,
Thank you for your great works. Attached is the latest version.
> Thanks! What about updating the comments furthermore as follows?
>
> -
> Use pgfdw_application_name as application_name if set.
>
> PQconnectdbParams() processes the parameter array
Dear Fujii-san,
Thank you for updating! Your modification is very interesting and
I learn something new.
> Attached is the updated version of the patch. I removed the test
> for case (1). And I arranged the regression tests so that they are based
> on debug_discard_caches, to simplify them. Also
Dear Ikeda-san,
> I agree that this feature is useful. Thanks for working this.
Thanks :-).
> 1)
>
> Why does postgres_fdw.application_name override the server's option?
>
> > + establishes a connection to a foreign server. This overrides
> > + application_name option of the server
Dear Fujii-san,
I confirmed it and I think it's OK.
Other comments should be included in 0002.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Dear Fujii-san, Ikeda-san,
> Pushed 0001 patch. Thanks!
I confirmed your commit. Thanks!
I attached the rebased version. Tests and descriptions were added.
In my understanding Ikeda-san's indication is included.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v10_0002_allow_escapes.patch
Descript
Dear Horiguchi-san,
Thank you for reviewing! I attached the fixed version.
> Is "the previous comment" "the comment above"?
Yeah, fixed.
> + for (i = n -1; i >= 0; i--)
>
> You might want a space between - and 1.
Fixed.
> +parse_application_name(StringInfo buf, const char *name)
Dear Fujii-san,
Thank you for reviewing!
> postgres_fdw gets out of the loop after processing appname even
> when buf.data is '\0'. Is this expected behavior? Because of this,
> when postgres_fdw.application_name = '%b', unprocessed appname
> of the server object is used.
In this case, I expecte
Dear Fujii-san,
Thank you for quick reviewing!
> We can simplify the code as follows.
>
> if (values[i] != '\0')
> break;
Fixed. And type mismatching was also fixed.
> IMO it's better to use process_padding() to process log_line_prefix and
> postgres_fdw.application in the same w
Dear Hou,
> I found one minor thing in the patch.
Thanks!
> Is it better to use Abs(padding) here ?
> Although some existing codes are using " padding > 0 ? padding : -padding ".
+1, fixed.
And I found that some comments were not added above padding calculation,
so added.
Best Regards,
Hayato
Dear Horiguchi-san,
Thank you for giving comments!
> Thanks for the new version. I don't object for reusing
> process_log_prefix_padding, but I still find it strange that the
> function with the name 'process_padding' is in string.c. If we move
> it to string.c, I'd like to name it "pg_fast_str
Dear Fujii-san,
Thanks for comments!
> >> Thanks for the new version. I don't object for reusing
> >> process_log_prefix_padding, but I still find it strange that the
> >> function with the name 'process_padding' is in string.c. If we move
> >> it to string.c, I'd like to name it "pg_fast_strto
Dear Fujii-san, Horiguchi-san,
Based on your advice, I made a patch
that communize two parsing functions into one.
new internal function parse_format_string() was added.
(This name may be too generic...)
log_line_prefix() and parse_pgfdw_appname() become just the wrapper function.
My prerpimary ch
Dear Fujii-san, Horiguchi-san
I'm sorry for sending a bad patch...
> ---
> elog.c:2785:14: warning: expression which evaluates to zero treated as a null
> pointer constant of type 'char *' [-Wnon-literal-null-conversion]
> *endptr = '\0';
>
Dear Horiguchi-san,
Thank you for giving many comments! I attached new patches.
I'm sorry for the late reply.
> I think we don't have a predecessor of the case like this where a
> behavior is decided from object option and GUC.
>
> I'm a bit uncomfortable with .conf configuration overrides serve
Dear Fujii-san,
Thank you for reviewing!
> + else if (*p == '-' ? isdigit(p[1]) : isdigit(p[0]))
> + {
> + char *endptr;
> + padding = strtol(p, &endptr, 10);
>
> Maybe isdigit() and strtol() work differently depending on locale set
Dear Hackers,
While reading source codes about timeouts and GUC and I found that
strange behavior about client_connection_check_interval.
Currently we did not an assign_hook about client_connection_check_interval,
that means a timeout will not turn on immediately if users change the GUC
from zero
Dear Horiguchi-san,
Thank you for replying! I understood I was wrong. Sorry.
> You're misunderstanding here. Maybe you saw that start_xact_command()
> starts the timer but note that the function is called before every
> command execution.
Based on your advice I read codes again and I found that
Hi Hackers,
I propose adding trackable information in postgres_fdw, in order to track
remote query correctly.
## Background and motivation
Currently postgres_fdw connects remote servers by using connect_pg_server().
However the function just calls PQconnectdbParams() with
fallback_application
Dear Tom,
Thank you for replying!
> I don't think this is a great idea as-is. People who need to do this
> sort of thing will all have their own ideas of what they need to track
> --- most obviously, it might be appropriate to include the originating
> server's name, else you don't know what mac
Dear Hackers, Tom,
(I changed subject because this is no longer postgres_fdw's patch)
> > What would be better to think about is how to let users specify this
> > kind of behavior for themselves. I think it's possible to set
> > application_name as part of a foreign server's connection options,
Dear Hackers,
I perfectly missed mails and 8/9 was a national holiday.
I must apologize about anything. Very sorry for inconvenience.
> The RMT's first responsibility is to ensure that PostgreSQL 14 is
> released on schedule. We would prefer to avoid a revert, but we cannot
> allow this to drag o
Dear Wang,
Good reporting!
> I'm not sure, but how about modify the ecpg.trailer:
> > statement: ecpgstart at toplevel_stmt ';' { connection = NULL; }
> > | ecpgstart toplevel_stmt ';'
> I think there are lots of 'connection = NULL;' in source, and we should find a
good location to reset the conn
Dear Horiguchi-san,
> How Pro*C behaves in that case? If the second command ends with an
> error, I think we are free to error out the second command before
> execution. If it works... do you know what is happening at the time?
You asked that how Oracle works when the followings precompiled and
e
Dear Meskes and any hackers,
> Yes, at least technically. I think DESCRIBE should accept the cached
> connection name, although it won't really matter.
I sought docs too and I found that Pro*C have such a same policy,
so it might be reasonable:
https://docs.oracle.com/en/database/oracle/oracle-d
Hi,
Sorry for being late. I had a vaccination.
I'm not sure about the rule that stderr should be removed
even if the pre-compiling state, but anyway I agree that
the warned case is not expected.
The wrong message is perfectly fault...
I confirmed your commit and I think it's OK. Thanks!
Best Re
Dear Wang,
Thank you for updating the patch! Followings are comments about v23-0001 and
v23-0005.
v23-0001
01. logical-replication.sgml
+
+ When the streaming mode is parallel, the finish LSN of
+ failed transactions may not be logged. In that case, it may be necessary to
+ change the
Dear Wang,
Followings are my comments about v23-0003. Currently I do not have any comments
about 0002 and 0004.
09. general
It seems that logicalrep_rel_mark_parallel_apply() is always called when
relations are opened on the subscriber-side,
but is it really needed? There checks are required o
Hi hackers,
While reading codes related with logical decoding,
I thought that following comment in rmgrlist.h is not consistent.
> /* symbol name, textual name, redo, desc, identify, startup, cleanup */
This comment describes a set of APIs that the resource manager should have, but
functions fo
> Your observation seems correct to me but you have not updated the
> comment for the mask. Is there a reason for the same?
Oh, it seems that I attached wrong one. There is no reason.
PSA the newer version.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v2-0001-add-a-missing-comment.patch
Descrip
Hi Hackers,
> Therefore, this leads to the failure for the assert that can check
> the consistency that when one sub transaction modifies the catalog,
> its top transaction should be marked so as well.
>
> I feel we need to remember the relationship between top transaction and sub
> transaction
>
Dear Horiguchi-san, Dilip,
Thank you for replying!
> > It seems that SnapBuildCommitTxn() is already taking care of adding
> > the top transaction to the committed transaction if any subtransaction
> > has the catalog changes, it has just missed setting the flag so I
> > think just setting the fl
> > I'm basically fine, too. But this is a bug that needs back-patching
> > back to 10.
> >
>
> I have not verified but I think we need to backpatch this till 14
> because prior to that in DecodeCommit, we use to set the top-level txn
> as having catalog changes based on the if there are invalidat
Dear hackers,
> I agreed both that DEBUG2 messages are still useful but we should not
> change the condition for output. So I prefer the idea suggested by Amit.
PSA newer patch, which contains the fix and test.
> > I have not verified but I think we need to backpatch this till 14
> > because pri
> I was not sure what's the proper way to fix it.
> The solution I've thought at first was transporting all invalidations from
> sub to top
> like ReorderBufferTransferSnapToParent(),
> but I do not know its side effect. Moreover, how do we deal with
> ReorderBufferChange?
> Should we transfer the
Dear Amit,
Thanks for giving comments!
> Did you get this new assertion failure after you applied the patch for
> the first failure? Because otherwise, how can you reach it with the
> same test case?
The first failure is occurred only in the HEAD, so I did not applied the first
patch
to REL14 a
Dear Hou-san,
Thank you for updating the patch! Followings are comments for v28-0001.
I will dig your patch more, but I send partially to keep the activity of the
thread.
===
For applyparallelworker.c
01. filename
The word-ordering of filename seems not good
because you defined the new worker a
Dear Hou-san,
> I will dig your patch more, but I send partially to keep the activity of the
> thread.
More minor comments about v28.
===
About 0002
For 015_stream.pl
14. check_parallel_log
```
+# Check the log that the streamed transaction was completed successfully
+# reported by parallel
Hi,
> > 01. filename
> > The word-ordering of filename seems not good
> > because you defined the new worker as "parallel apply worker".
> >
>
> I think in the future we may have more files for apply work (like
> applyddl.c for DDL apply work), so it seems okay to name all apply
> related files i
Hi,
Sorry for noise but I found another bug.
When the 032_apply_delay.pl is modified like following,
the test will be always failed even if my patch is applied.
```
# Disable subscription. worker should die immediately.
-$node_subscriber->safe_psql('postgres',
- "ALTER SUBSCRIPTION tap_sub
Dear Önder,
Thank you for proposing good feature. I'm also interested in the patch,
So I started to review this. Followings are initial comments.
===
For execRelation.c
01. RelationFindReplTupleByIndex()
```
/* Start an index scan. */
InitDirtySnapshot(snap);
- scan = in
Dear Önder,
Thanks for updating the patch! I will check it later.
Currently I just reply to your comments.
> Hmm, I couldn't realize this comment earlier. So you suggest "slow" here
> refers to the additional function call "GetRelationIdentityOrPK"? If so, yes
> I'll update that.
Yes I meant t
> One last thing - do you think there is any need to mention this
> behaviour in the pgdocs, or is OK just to be a hidden performance
> improvement?
FYI - I put my opinion.
We have following sentence in the logical-replication.sgml:
```
...
If the table does not have any suitable key, then it can
Dear Fujii-san,
Thanks for checking!
> These failed to be applied to the master branch cleanly. Could you update
> them?
PSA rebased patches. I reviewed my myself and they contain changes.
E.g., move GUC-related code to option.c.
> + this option relies on kernel events exposed by Linux,
Dear Wang,
Thanks for updating the patch! Followings are comments for v33-0001.
===
libpqwalreceiver.c
01. inclusion
```
+#include "catalog/pg_subscription.h"
```
We don't have to include it because the analysis of parameters is done at
caller.
===
launcher.c
02. logicalrep_worker_launch()
101 - 200 of 207 matches
Mail list logo
