Re: [Code Cleanup] : Small code cleanup in twophase.sql
Hi, Any taker or rejector for above? -- It's a very small 'good to have' change patch for cleanup. Thanks, Nishant (EDB). On Tue, Sep 26, 2023 at 6:31 PM Nishant Sharma < nishant.sha...@enterprisedb.com> wrote: > Hi, > > > PFA small code cleanup in twophase.sql. Which contains a drop table > statement for 'test_prepared_savepoint'. Which, to me, appears to be > missing in the cleanup section of that file. > > To support it I have below points:- > > 1) Grepping this table 'test_prepared_savepoint' shows occurrences > only in twophase.out & twophase.sql files. This means that table is > local to that sql test file and not used in any other test file. > > 2) I don't see any comment on why this was not added in the cleanup > section of twophase.sql, but drop for other two test tables are done. > > 3) I ran "make check-world" with the patch and I don't see any failures. > > Kindly correct, if I missed anything. > > > Regards, > Nishant (EDB). >
[Code Cleanup] : Small code cleanup in twophase.sql
Hi, PFA small code cleanup in twophase.sql. Which contains a drop table statement for 'test_prepared_savepoint'. Which, to me, appears to be missing in the cleanup section of that file. To support it I have below points:- 1) Grepping this table 'test_prepared_savepoint' shows occurrences only in twophase.out & twophase.sql files. This means that table is local to that sql test file and not used in any other test file. 2) I don't see any comment on why this was not added in the cleanup section of twophase.sql, but drop for other two test tables are done. 3) I ran "make check-world" with the patch and I don't see any failures. Kindly correct, if I missed anything. Regards, Nishant (EDB). v1-0001-Small-code-cleanup-in-twophase.sql.patch Description: Binary data
Re: pg_basebackup: Always return valid temporary slot names
On Tue, Sep 5, 2023 at 4:40 PM Jelte Fennema wrote: > I modified the PgBouncer PR to always set the sign bit to 0. But I > still I think it makes sense to also address this in pg_basebackup. Sounds good to me. Thank you! On Tue, Sep 5, 2023 at 5:36 PM Daniel Gustafsson wrote: > Since the value in the temporary slotname isn't used to convey meaning, but > merely to ensure uniqueness, I don't think it's unreasonable to guard > aginst > malformed input (ie negative integer). > Ok. In this case, I also agree. +1 to the patch from my side. Thank you! Regards, Nishant.
Re: pg_basebackup: Always return valid temporary slot names
Hi Jelte, Please find my reviews below:- *1)* With what I have understood from above, the pgbouncer fills up be_pid (int, 32 bits) with random bits as it does not have an associated server connection yet. With this, I was thinking, isn't this a problem of pgbouncer filling be_pid with random bits? Maybe it should have filled the be_pid with a random positive integer instead of any integer as it represents a pid? -- If this makes sense here, then maybe the fix should be in pgbouncer instead of how the be_pid is processed in pg_basebackup? *2)* Rest, the patch looks straightforward, with these two changes : "%d" --> "%u" and "(int)" --> "(unsigned)". Regards, Nishant. On Thu, Aug 31, 2023 at 2:43 PM Jelte Fennema wrote: > With PgBouncer in the middle PQbackendPID can return negative values > due to it filling all 32 bits of the be_pid with random bits. > > When this happens it results in pg_basebackup generating an invalid slot > name (when no specific slot name is passed in) and thus throwing an > error like this: > > pg_basebackup: error: could not send replication command > "CREATE_REPLICATION_SLOT "pg_basebackup_-1201966863" TEMPORARY > PHYSICAL ( RESERVE_WAL)": ERROR: replication slot name > "pg_basebackup_-1201966863" contains invalid character > HINT: Replication slot names may only contain lower case letters, > numbers, and the underscore character. > > This patch fixes that problem by formatting the result from PQbackendPID > as an unsigned integer when creating the temporary replication slot > name. > > I think it would be good to backport this fix too. > > Replication connection support for PgBouncer is not merged yet, but > it's pretty much ready: > https://github.com/pgbouncer/pgbouncer/pull/876 > > The reason PgBouncer does not pass on the actual Postgres backend PID > is that it doesn't have an associated server connection yet when it > needs to send the startup message to the client. It also cannot use > it's own PID, because that would be the same for all clients, since > pgbouncer is a single process. >
Re: postgres_fdw: wrong results with self join + enable_nestloop off
Looks good to me. Tested on master and it works. New patch used a bool flag to avoid calls for both FDW and custom hook's call. And a slight change in comment of "has_pseudoconstant_clauses" function. Regards, Nishant. On Wed, Jun 14, 2023 at 12:19 PM Etsuro Fujita wrote: > On Mon, Jun 5, 2023 at 10:19 PM Etsuro Fujita > wrote: > > To avoid this issue, I am wondering if we should modify > > add_paths_to_joinrel() in back branches so that it just disallows the > > FDW to consider pushing down joins when the restrictlist has > > pseudoconstant clauses. Attached is a patch for that. > > I think that custom scans have the same issue, so I modified the patch > further so that it also disallows custom-scan providers to consider > join pushdown in add_paths_to_joinrel() if necessary. Attached is a > new version of the patch. > > Best regards, > Etsuro Fujita >
Re: postgres_fdw: wrong results with self join + enable_nestloop off
Hi, Etsuro's patch is also showing the correct output for "set enable_nestloop=off". Looks good to me for back branches due to backport issues. But below are a few observations for the same:- 1) I looked into the query plan for both "set enable_nestloop" on & off case and observe that they are the same. That is, what we see with "set enable_nestloop=on". 2) In back branches for "set enable_nestloop" on & off value, at least this type of query execution won't make any difference. No comparison of plans to be selected based on total cost of two plans old (Nested Loop with Foreign Scans) & new (Only Foreign Scan) will be done, because we are avoiding the call to "postgresGetForeignJoinPaths()" up front when we have pseudo constants. Regards, Nishant. On Tue, Jun 6, 2023 at 8:50 AM Richard Guo wrote: > > On Mon, Jun 5, 2023 at 9:19 PM Etsuro Fujita > wrote: > >> If the patch is intended for HEAD only, I also think it goes in the >> right direction. But if it is intended for back branches as well, I >> do not think so, because it would cause ABI breakage due to changes >> made to the ForeignPath struct and the create_foreign_join_path() API. >> (For the former, I think we could avoid doing so by adding the new >> member at the end of the struct, not in the middle, though.) > > > Thanks for pointing this out. You're right. The patch has backport > issue because of the ABI breakage. So it can only be applied on HEAD. > > >> To avoid this issue, I am wondering if we should modify >> add_paths_to_joinrel() in back branches so that it just disallows the >> FDW to consider pushing down joins when the restrictlist has >> pseudoconstant clauses. Attached is a patch for that. > > > I think we can do that in back branches. But I'm a little concerned > that we'd miss a better plan if FDW cannot push down joins in such > cases. I may be worrying over nothing though if it's not common that > the restrictlist has pseudoconstant clauses. > > Thanks > Richard >
Re: postgres_fdw: wrong results with self join + enable_nestloop off
I also agree that Richard's patch is better. As it fixes the issue at the backend and does not treat pseudoconstant as local condition. I have tested Richard's patch and observe that it is resolving the problem. Patch looks good to me as well. *I only had a minor comment on below change:-* *- gating_clauses = get_gating_quals(root, scan_clauses);+ if (best_path->pathtype == T_ForeignScan && IS_JOIN_REL(rel))+ gating_clauses = get_gating_quals(root, ((ForeignPath *) best_path)->joinrestrictinfo);+ else+ gating_clauses = get_gating_quals(root, scan_clauses);* >> Instead of using 'if' and creating a special case here can't we do something in the above switch? Regards, Nishant. P.S I tried something quickly but I am seeing a crash:- *case T_IndexOnlyScan:scan_clauses = castNode(IndexPath, best_path)->indexinfo->indrestrictinfo; break;+ case T_ForeignScan:+ /*+* Note that for scans of foreign joins, we do not have restriction clauses+* stored in baserestrictinfo and we do not consider parameterization.+ * Instead we need to check against joinrestrictinfo stored in ForeignPath.+*/+ if (IS_JOIN_REL(rel))+ scan_clauses = ((ForeignPath *) best_path)->joinrestrictinfo;+ else+ scan_clauses = rel->baserestrictinfo;+ break;default: scan_clauses = rel->baserestrictinfo;break;* On Fri, Jun 2, 2023 at 9:00 AM Suraj Kharage wrote: > +1 for fixing this in the backend code rather than FDW code. > > Thanks, Richard, for working on this. The patch looks good to me at > a glance. > > On Tue, Apr 25, 2023 at 3:36 PM Richard Guo > wrote: > >> >> On Fri, Apr 14, 2023 at 8:51 PM Etsuro Fujita >> wrote: >> >>> I think that the root cause for this issue would be in the >>> create_scan_plan handling of pseudoconstant quals when creating a >>> foreign-join (or custom-join) plan. >> >> >> Yes exactly. In create_scan_plan, we are supposed to extract all the >> pseudoconstant clauses and use them as one-time quals in a gating Result >> node. Currently we check against rel->baserestrictinfo and ppi_clauses >> for the pseudoconstant clauses. But for scans of foreign joins, we do >> not have any restriction clauses in these places and thus the gating >> Result node as well as the pseudoconstant clauses would just be lost. >> >> I looked at Nishant's patch. IIUC it treats the pseudoconstant clauses >> as local conditions. While it can fix the wrong results issue, I think >> maybe it's better to still treat the pseudoconstant clauses as one-time >> quals in a gating node. So I wonder if we can store the restriction >> clauses for foreign joins in ForeignPath, just as what we do for normal >> JoinPath, and then check against them for pseudoconstant clauses in >> create_scan_plan, something like attached. >> >> BTW, while going through the codes I noticed one place in >> add_foreign_final_paths that uses NULL for List *. I changed it to NIL. >> >> Thanks >> Richard >> > > > -- > -- > > Thanks & Regards, > Suraj kharage, > > > > edbpostgres.com >
Re: postgres_fdw: wrong results with self join + enable_nestloop off
Hi Etsuro Fujita, Any updates? -- did you get a chance to look into this? Regards, Nishant. On Mon, Apr 17, 2023 at 11:00 AM Nishant Sharma < nishant.sha...@enterprisedb.com> wrote: > Thanks Etsuro for your response! > > One small typo correction in my answer to "What is the technical issue?" > "it is *not* considered a pseudo constant" --> "it is considered a pseudo > constant" > > > Regards, > Nishant. > > On Fri, Apr 14, 2023 at 6:21 PM Etsuro Fujita > wrote: > >> Hi Nishant, >> >> On Fri, Apr 14, 2023 at 8:39 PM Nishant Sharma >> wrote: >> > I debugged this issue and was able to find a fix for the same. Kindly >> please refer to the attached fix. With the fix I am able to resolve the >> issue. >> >> Thanks for the report and patch! >> >> > What is the technical issue? >> > The problem here is the use of extract_actual_clauses. Because of which >> the plan creation misses adding the second condition of AND i.e "now() < >> '23-Feb-2020'::timestamp" in the plan. Because it is not considered a >> pseudo constant and extract_actual_clause is passed with false as the >> second parameter and it gets skipped from the list. As a result that >> condition is never taken into consideration as either one-time filter >> (before or after) or part of SQL remote execution >> > >> > Why do I think the fix is correct? >> > The fix is simple, where we have created a new function similar to >> extract_actual_clause which just extracts all the conditions from the list >> with no checks and returns the list to the caller. As a result all >> conditions would be taken into consideration in the query plan. >> >> I think that the root cause for this issue would be in the >> create_scan_plan handling of pseudoconstant quals when creating a >> foreign-join (or custom-join) plan. Anyway, I will look at your patch >> closely, first. >> >> Best regards, >> Etsuro Fujita >> >
Re: postgres_fdw: wrong results with self join + enable_nestloop off
Thanks Etsuro for your response! One small typo correction in my answer to "What is the technical issue?" "it is *not* considered a pseudo constant" --> "it is considered a pseudo constant" Regards, Nishant. On Fri, Apr 14, 2023 at 6:21 PM Etsuro Fujita wrote: > Hi Nishant, > > On Fri, Apr 14, 2023 at 8:39 PM Nishant Sharma > wrote: > > I debugged this issue and was able to find a fix for the same. Kindly > please refer to the attached fix. With the fix I am able to resolve the > issue. > > Thanks for the report and patch! > > > What is the technical issue? > > The problem here is the use of extract_actual_clauses. Because of which > the plan creation misses adding the second condition of AND i.e "now() < > '23-Feb-2020'::timestamp" in the plan. Because it is not considered a > pseudo constant and extract_actual_clause is passed with false as the > second parameter and it gets skipped from the list. As a result that > condition is never taken into consideration as either one-time filter > (before or after) or part of SQL remote execution > > > > Why do I think the fix is correct? > > The fix is simple, where we have created a new function similar to > extract_actual_clause which just extracts all the conditions from the list > with no checks and returns the list to the caller. As a result all > conditions would be taken into consideration in the query plan. > > I think that the root cause for this issue would be in the > create_scan_plan handling of pseudoconstant quals when creating a > foreign-join (or custom-join) plan. Anyway, I will look at your patch > closely, first. > > Best regards, > Etsuro Fujita >
postgres_fdw: wrong results with self join + enable_nestloop off
stamp; QUERY PLAN - Foreign Scan (cost=100.00..49310.40 rows=2183680 width=16) (actual time=0.652..0.652 rows=0 loops=1) Output: tbl1.id1, tbl1.id2, tbl2.id1, tbl2.id2 Filter: (now() < '2020-02-23 00:00:00'::timestamp without time zone) Rows Removed by Filter: 9 Relations: (public.pg_tbl_foreign tbl1) INNER JOIN (public.pg_tbl_foreign tbl2) Remote SQL: SELECT r1.id1, r1.id2, r2.id1, r2.id2 FROM (public.pg_tbl r1 INNER JOIN public.pg_tbl r2 ON (((r1.id1 < 5 Planning Time: 0.133 ms Execution Time: 1.127 ms (8 rows) postgres@78754=#set enable_nestloop=on; SET postgres@78754=#select * from pg_tbl_foreign tbl1 join pg_tbl_foreign tbl2 on tbl1.id1 < 5 and now() < '23-Feb-2020'::timestamp; id1 | id2 | id1 | id2 -+-+-+- (0 rows) postgres@78754=#explain (analyze, verbose) select * from pg_tbl_foreign tbl1 join pg_tbl_foreign tbl2 on tbl1.id1 < 5 and now() < '23-Feb-2020'::timestamp; QUERY PLAN --- Result (cost=200.00..27644.00 rows=2183680 width=16) (actual time=0.004..0.005 rows=0 loops=1) Output: tbl1.id1, tbl1.id2, tbl2.id1, tbl2.id2 One-Time Filter: (now() < '2020-02-23 00:00:00'::timestamp without time zone) -> Nested Loop (cost=200.00..27644.00 rows=2183680 width=16) (never executed) Output: tbl1.id1, tbl1.id2, tbl2.id1, tbl2.id2 -> Foreign Scan on public.pg_tbl_foreign tbl2 (cost=100.00..186.80 rows=2560 width=8) (never executed) Output: tbl2.id1, tbl2.id2 Remote SQL: SELECT id1, id2 FROM public.pg_tbl -> Materialize (cost=100.00..163.32 rows=853 width=8) (never executed) Output: tbl1.id1, tbl1.id2 -> Foreign Scan on public.pg_tbl_foreign tbl1 (cost=100.00..159.06 rows=853 width=8) (never executed) Output: tbl1.id1, tbl1.id2 Remote SQL: SELECT id1, id2 FROM public.pg_tbl WHERE ((id1 < 5)) Planning Time: 0.134 ms Execution Time: 0.347 ms (15 rows) |************| Kindly please comment if I am in the correct direction or not? Regards, Nishant Sharma. Developer at EnterpriseDB, Pune, India. P.S Steps that I used to create local postgres FDW setup ( followed link - https://www.postgresql.org/docs/current/postgres-fdw.html <https://www.postgresql.org/docs/current/postgres-fdw.html):-> ) 1) ./configure --prefix=/home/edb/POSTGRES_INSTALL/MASTER --with-pgport=9996 --with-openssl --with-libxml --with-zlib --with-tcl --with-perl --with-libxslt --with-ossp-uuid --with-ldap --with-pam --enable-nls --enable-debug --enable-depend --enable-dtrace --with-selinux --with-icu --enable-tap-tests --enable-cassert CFLAGS="-g -O0" 2) make 3) make install 4) cd contrib/postgres_fdw/ 5) make install 6) Start the server 7) [edb@localhost MASTER]$ bin/psql postgres edb; psql (16devel) Type "help" for help. postgres@70613=#create database remote_db; CREATE DATABASE postgres@70613=#quit [edb@localhost MASTER]$ bin/psql remote_db edb; psql (16devel) Type "help" for help. remote_db@70613=#CREATE USER fdw_user; CREATE ROLE remote_db@70613=#GRANT ALL ON SCHEMA public TO fdw_user; GRANT remote_db@70613=#quit [edb@localhost MASTER]$ bin/psql remote_db fdw_user; psql (16devel) Type "help" for help. remote_db@70613=#create table pg_tbl(id1 int, id2 int); CREATE TABLE remote_db@70613=#insert into pg_tbl values(1, 10); INSERT 0 1 remote_db@70613=#insert into pg_tbl values(2, 20); INSERT 0 1 remote_db@70613=#insert into pg_tbl values(3, 30); INSERT 0 1 8) New terminal/Tab:- [edb@localhost MASTER]$ bin/psql postgres edb; postgres@71609=#create extension postgres_fdw; CREATE EXTENSION postgres@71609=#CREATE SERVER localhost_fdw FOREIGN DATA WRAPPER postgres_fdw OPTIONS (dbname 'remote_db', host 'localhost', port '9996'); CREATE SERVER postgres@71609=#CREATE USER MAPPING for edb SERVER localhost_fdw OPTIONS (user 'fdw_user', password ''); CREATE USER MAPPING postgres@71609=#GRANT ALL ON FOREIGN SERVER localhost_fdw TO edb; GRANT postgres@71609=#CREATE FOREIGN TABLE pg_tbl_foreign(id1 int, id2 int) SERVER localhost_fdw OPTIONS (schema_name 'public', table_name 'pg_tbl'); CREATE FOREIGN TABLE postgres@71609=#select * from pg_tbl_foreign; id1 | id2 -+- 1 | 10 2 | 20 3 | 30 (3 rows) PG_PATCH_set_nestloop_off_issue_fix.patch Description: Binary data
[PROPOSAL] : Use of ORDER BY clause in insert.sql
Hi, We would like to share a proposal of a patch, where we have added order by clause in two select statements in src/test/regress/sql/insert.sql file and respective changes in src/test/regress/expected/insert.out output file. This would help in generating output in consistent sequence, as sometimes we have observed change in sequence in output. Please find the patch attached Regards, Nishant Sharma EDB: http://www.enterprisedb.com Proposal_OrderBy_insert.sql.out.patch Description: Binary data
