public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v16 02/10] pg_stat_file and pg_ls_dir_* to use lstat().. 5+ messages / 4 participants [nested] [flat]
* [PATCH v16 02/10] pg_stat_file and pg_ls_dir_* to use lstat().. @ 2020-03-30 23:59 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Justin Pryzby @ 2020-03-30 23:59 UTC (permalink / raw) pg_ls_dir_* will now skip (no longer show) symbolic links, same as other non-regular file types, as we advertize we do since 8b6d94cf6. That seems to be the intented behavior, since irregular file types are 1) less portable; and, 2) we don't currently show a file's type except for "bool is_dir". pg_stat_file will now 1) show metadata of links themselves, rather than their target; and, 2) specifically, show links to directories with "is_dir=false"; and, 3) not error on broken symlinks. --- doc/src/sgml/func.sgml | 2 +- src/backend/utils/adt/genfile.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 9b885102da..96b08d0500 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25486,7 +25486,7 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8'); size, last accessed time stamp, last modified time stamp, last file status change time stamp (Unix platforms only), file creation time stamp (Windows only), and a <type>boolean</type> - indicating if it is a directory (or a symbolic link to a directory). + indicating if it is a directory. Typical usages include: <programlisting> SELECT * FROM pg_stat_file('filename'); diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index ceaa6180da..219ac160f8 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -370,7 +370,7 @@ pg_stat_file(PG_FUNCTION_ARGS) filename = convert_and_check_filename(filename_t); - if (stat(filename, &fst) < 0) + if (lstat(filename, &fst) < 0) { if (missing_ok && errno == ENOENT) PG_RETURN_NULL(); @@ -596,7 +596,7 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok) /* Get the file info */ snprintf(path, sizeof(path), "%s/%s", dir, de->d_name); - if (stat(path, &attrib) < 0) + if (lstat(path, &attrib) < 0) { /* Ignore concurrently-deleted files, else complain */ if (errno == ENOENT) -- 2.17.0 --2FkSFaIQeDFoAt0B Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v16-0003-Add-tests-on-pg_ls_dir-before-changing-it.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Wrong results from Parallel Hash Full Join @ 2023-04-12 22:49 Thomas Munro <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Thomas Munro @ 2023-04-12 22:49 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Andres Freund <[email protected]>; Richard Guo <[email protected]>; pgsql-hackers On Thu, Apr 13, 2023 at 9:48 AM Melanie Plageman <[email protected]> wrote: > Attached patch includes the fix for ExecParallelHashTableInsert() as > well as a test. I toyed with adapting one of the existing parallel full > hash join tests to cover this case, however, I think Richard's repro is > much more clear. Maybe it is worth throwing in a few updates to the > tables in the existing queries to provide coverage for the other > HeapTupleHeaderClearMatch() calls in the code, though. Oof. Analysis and code LGTM. I thought about the way non-parallel HJ also clears the match bits when re-using the hash table for rescans. PHJ doesn't keep hash tables across rescans. (There's no fundamental reason why it couldn't, but there was some complication and it seemed absurd to have NestLoop over Gather over PHJ, forking a new set of workers for every tuple, so I didn't implement that in the original PHJ.) But... there is something a little odd about the code in ExecHashTableResetMatchFlags(), or the fact that we appear to be calling it: it's using the unshared union member unconditionally, which wouldn't actually work for PHJ (there should be a variant of that function with Parallel in its name if we ever want that to work). That's not a bug AFAICT, as in fact we don't actually call it--it should be unreachable because the hash table should be gone when we rescan--but it's confusing. I'm wondering if we should put in something explicit about that, maybe a comment and an assertion in ExecReScanHashJoin(). +-- Ensure that hash join tuple match bits have been cleared before putting them +-- into the hashtable. Could you mention that the match flags steals a bit from the HOT flag, ie *why* we're testing a join after an update? And if we're going to exercise/test that case, should we do the non-parallel version too? For the commit message, I think it's a good idea to use something like "Fix ..." for the headline of bug fix commits to make that clearer, and to add something like "oversight in commit XYZ" in the body, just to help people connect the dots. (Yeah, I know I failed to reference the delinquent commit in the recent assertion-removal commit, my bad.) I think "Discussion:" footers are supposed to use https://postgr.es/m/XXX shortened URLs. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Wrong results from Parallel Hash Full Join @ 2023-04-13 00:31 Melanie Plageman <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Melanie Plageman @ 2023-04-13 00:31 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Andres Freund <[email protected]>; Richard Guo <[email protected]>; pgsql-hackers On Wed, Apr 12, 2023 at 6:50 PM Thomas Munro <[email protected]> wrote: > > On Thu, Apr 13, 2023 at 9:48 AM Melanie Plageman > <[email protected]> wrote: > > Attached patch includes the fix for ExecParallelHashTableInsert() as > > well as a test. I toyed with adapting one of the existing parallel full > > hash join tests to cover this case, however, I think Richard's repro is > > much more clear. Maybe it is worth throwing in a few updates to the > > tables in the existing queries to provide coverage for the other > > HeapTupleHeaderClearMatch() calls in the code, though. > > Oof. Analysis and code LGTM. > > I thought about the way non-parallel HJ also clears the match bits > when re-using the hash table for rescans. PHJ doesn't keep hash > tables across rescans. (There's no fundamental reason why it > couldn't, but there was some complication and it seemed absurd to have > NestLoop over Gather over PHJ, forking a new set of workers for every > tuple, so I didn't implement that in the original PHJ.) But... there > is something a little odd about the code in > ExecHashTableResetMatchFlags(), or the fact that we appear to be > calling it: it's using the unshared union member unconditionally, > which wouldn't actually work for PHJ (there should be a variant of > that function with Parallel in its name if we ever want that to work). > That's not a bug AFAICT, as in fact we don't actually call it--it > should be unreachable because the hash table should be gone when we > rescan--but it's confusing. I'm wondering if we should put in > something explicit about that, maybe a comment and an assertion in > ExecReScanHashJoin(). An assert about it not being a parallel hash join? I support this. > +-- Ensure that hash join tuple match bits have been cleared before putting them > +-- into the hashtable. > > Could you mention that the match flags steals a bit from the HOT flag, > ie *why* we're testing a join after an update? v2 attached has some wordsmithing along these lines. > And if we're going to > exercise/test that case, should we do the non-parallel version too? I've added this. I thought if we were adding the serial case, we might as well add the multi-batch case as well. However, that proved a bit more challenging. We can get a HOT tuple in one of the existing tables with no issues. Doing this and then deleting the reset match bit code doesn't cause any of the tests to fail, however, because we use this expression as the join condition when we want to emit NULL-extended unmatched tuples. select count(*) from simple r full outer join simple s on (r.id = 0 - s.id); I don't think we want to add yet another time-consuming test to this test file. So, I was trying to decide if it was worth changing these existing tests so that they would fail when the match bit wasn't reset. I'm not sure. > For the commit message, I think it's a good idea to use something like > "Fix ..." for the headline of bug fix commits to make that clearer, > and to add something like "oversight in commit XYZ" in the body, just > to help people connect the dots. (Yeah, I know I failed to reference > the delinquent commit in the recent assertion-removal commit, my bad.) I've made these edits and tried to improve the commit message clarity in general. > I think "Discussion:" footers are supposed to use > https://postgr.es/m/XXX shortened URLs. Hmm. Is the problem with mine that I included "flat"? Because I did use postgr.es/m format. The message id is unfortunately long, but I believe that is on google and not me. - Melanie Attachments: [text/x-patch] v2-0001-Fix-PHJ-tuple-match-bit-management.patch (5.1K, ../../CAAKRu_av+NCVPXkLLTBG2VZcAk83FrzR9z-kZ-NQ+1kai9Tibw@mail.gmail.com/2-v2-0001-Fix-PHJ-tuple-match-bit-management.patch) download | inline diff: From ee24229a1eeca67dfd91e162502efe2abfead870 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Wed, 12 Apr 2023 17:16:28 -0400 Subject: [PATCH v2] Fix PHJ tuple match bit management Hash join tuples reuse the HOT status bit to indicate match status during hash join execution. Correct reuse requires clearing the bit in all tuples. Serial hash join and parallel multi-batch hash join do so upon inserting the tuple into the hashtable. Single batch parallel hash join and batch 0 of unexpected multi-batch hash joins forgot to do this. It hadn't come up before because hashtable tuple match bits are only used for right and full outer joins and parallel ROJ and FOJ were unsupported. 11c2d6fdf5 introduced support for parallel ROJ/FOJ but neglected to ensure the match bits were reset. Reported-by: Richard Guo Discussion: https://postgr.es/m/flat/CAMbWs48Nde1Mv%3DBJv6_vXmRKHMuHZm2Q_g4F6Z3_pn%2B3EV6BGQ%40mail.gmail.com --- src/backend/executor/nodeHash.c | 1 + src/test/regress/expected/join_hash.out | 37 +++++++++++++++++++++++++ src/test/regress/sql/join_hash.sql | 28 ++++++++++++++++++- 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c index a45bd3a315..54c06c5eb3 100644 --- a/src/backend/executor/nodeHash.c +++ b/src/backend/executor/nodeHash.c @@ -1724,6 +1724,7 @@ retry: /* Store the hash value in the HashJoinTuple header. */ hashTuple->hashvalue = hashvalue; memcpy(HJTUPLE_MINTUPLE(hashTuple), tuple, tuple->t_len); + HeapTupleHeaderClearMatch(HJTUPLE_MINTUPLE(hashTuple)); /* Push it onto the front of the bucket's list */ ExecParallelHashPushTuple(&hashtable->buckets.shared[bucketno], diff --git a/src/test/regress/expected/join_hash.out b/src/test/regress/expected/join_hash.out index 09376514bb..e892e7cccb 100644 --- a/src/test/regress/expected/join_hash.out +++ b/src/test/regress/expected/join_hash.out @@ -955,6 +955,43 @@ $$); (1 row) rollback to settings; +-- Hash join reuses the HOT status bit to indicate match status. This can only +-- be guaranteed to produce correct results if all the hash join tuple match +-- bits are reset before reuse. This is done upon loading them into the +-- hashtable. +SAVEPOINT settings; +SET enable_parallel_hash = on; +SET min_parallel_table_scan_size = 0; +SET parallel_setup_cost = 0; +SET parallel_tuple_cost = 0; +CREATE TABLE hjtest_matchbits_t1(id int); +CREATE TABLE hjtest_matchbits_t2(id int); +INSERT INTO hjtest_matchbits_t1 VALUES (1); +INSERT INTO hjtest_matchbits_t2 VALUES (2); +-- Update should create a HOT tuple. If this status bit isn't cleared, we won't +-- correctly emit the NULL-extended unmatching tuple in full hash join. +UPDATE hjtest_matchbits_t2 set id = 2; +SELECT * FROM hjtest_matchbits_t1 t1 FULL JOIN hjtest_matchbits_t2 t2 ON t1.id = t2.id; + id | id +----+---- + 1 | + | 2 +(2 rows) + +-- Test serial full hash join. +-- Resetting parallel_setup_cost should force a serial plan. +-- Just to be safe, however, set enable_parallel_hash to off, as parallel full +-- hash joins are only supported with shared hashtables. +RESET parallel_setup_cost; +SET enable_parallel_hash = off; +SELECT * FROM hjtest_matchbits_t1 t1 FULL JOIN hjtest_matchbits_t2 t2 ON t1.id = t2.id; + id | id +----+---- + 1 | + | 2 +(2 rows) + +ROLLBACK TO settings; rollback; -- Verify that hash key expressions reference the correct -- nodes. Hashjoin's hashkeys need to reference its outer plan, Hash's diff --git a/src/test/regress/sql/join_hash.sql b/src/test/regress/sql/join_hash.sql index 179e94941c..06bab7a4c7 100644 --- a/src/test/regress/sql/join_hash.sql +++ b/src/test/regress/sql/join_hash.sql @@ -506,8 +506,34 @@ $$ $$); rollback to settings; -rollback; +-- Hash join reuses the HOT status bit to indicate match status. This can only +-- be guaranteed to produce correct results if all the hash join tuple match +-- bits are reset before reuse. This is done upon loading them into the +-- hashtable. +SAVEPOINT settings; +SET enable_parallel_hash = on; +SET min_parallel_table_scan_size = 0; +SET parallel_setup_cost = 0; +SET parallel_tuple_cost = 0; +CREATE TABLE hjtest_matchbits_t1(id int); +CREATE TABLE hjtest_matchbits_t2(id int); +INSERT INTO hjtest_matchbits_t1 VALUES (1); +INSERT INTO hjtest_matchbits_t2 VALUES (2); +-- Update should create a HOT tuple. If this status bit isn't cleared, we won't +-- correctly emit the NULL-extended unmatching tuple in full hash join. +UPDATE hjtest_matchbits_t2 set id = 2; +SELECT * FROM hjtest_matchbits_t1 t1 FULL JOIN hjtest_matchbits_t2 t2 ON t1.id = t2.id; +-- Test serial full hash join. +-- Resetting parallel_setup_cost should force a serial plan. +-- Just to be safe, however, set enable_parallel_hash to off, as parallel full +-- hash joins are only supported with shared hashtables. +RESET parallel_setup_cost; +SET enable_parallel_hash = off; +SELECT * FROM hjtest_matchbits_t1 t1 FULL JOIN hjtest_matchbits_t2 t2 ON t1.id = t2.id; +ROLLBACK TO settings; + +rollback; -- Verify that hash key expressions reference the correct -- nodes. Hashjoin's hashkeys need to reference its outer plan, Hash's -- 2.37.2 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Wrong results from Parallel Hash Full Join @ 2023-04-13 23:05 Thomas Munro <[email protected]> parent: Melanie Plageman <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Thomas Munro @ 2023-04-13 23:05 UTC (permalink / raw) To: Melanie Plageman <[email protected]>; +Cc: Andres Freund <[email protected]>; Richard Guo <[email protected]>; pgsql-hackers On Thu, Apr 13, 2023 at 12:31 PM Melanie Plageman <[email protected]> wrote: > On Wed, Apr 12, 2023 at 6:50 PM Thomas Munro <[email protected]> wrote: > > I think "Discussion:" footers are supposed to use > > https://postgr.es/m/XXX shortened URLs. > > Hmm. Is the problem with mine that I included "flat"? Because I did use > postgr.es/m format. The message id is unfortunately long, but I believe > that is on google and not me. For some reason I thought we weren't supposed to use the flat thing, but it looks like I'm just wrong and people do that all the time so I take that back. Pushed. Thanks Richard and Melanie. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Wrong results from Parallel Hash Full Join @ 2023-04-14 12:37 Robert Haas <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Robert Haas @ 2023-04-14 12:37 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Andres Freund <[email protected]>; Richard Guo <[email protected]>; pgsql-hackers On Thu, Apr 13, 2023 at 7:06 PM Thomas Munro <[email protected]> wrote: > For some reason I thought we weren't supposed to use the flat thing, > but it looks like I'm just wrong and people do that all the time so I > take that back. > > Pushed. Thanks Richard and Melanie. I tend to use http://postgr.es/m/ or https://postgr.es/m/ just to keep the URL a bit shorter, and also because I like to point anyone reading the commit log to the particular message that I think is most relevant rather than to the thread as a whole. But I don't think there's any hard-and-fast rule that committers have to do it one way rather than another. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-04-14 12:37 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-30 23:59 [PATCH v16 02/10] pg_stat_file and pg_ls_dir_* to use lstat().. Justin Pryzby <[email protected]> 2023-04-12 22:49 Re: Wrong results from Parallel Hash Full Join Thomas Munro <[email protected]> 2023-04-13 00:31 ` Re: Wrong results from Parallel Hash Full Join Melanie Plageman <[email protected]> 2023-04-13 23:05 ` Re: Wrong results from Parallel Hash Full Join Thomas Munro <[email protected]> 2023-04-14 12:37 ` Re: Wrong results from Parallel Hash Full Join Robert Haas <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox