Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-11-13 Thread Tom Lane
Alexander Lakhin writes: > 13.11.2020 23:16, Tom Lane wrote: >> That would soon lead us to changing every stat() caller in the system >> to have Windows-specific looping logic. No thanks. If we need to do >> this, let's put in a Windows wrapper layer comparable to pgwin32_open() >> for open().

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-11-13 Thread Alexander Lakhin
13.11.2020 23:16, Tom Lane wrote: > Alexander Lakhin writes: >> Shouldn't pg_ls_dir_files() retry stat() on ERROR_ACCESS_DENIED just >> like the pgwin32_open() does to ignore files in "delete pending" state? > That would soon lead us to changing every stat() caller in the system > to have

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-11-13 Thread Tom Lane
Alexander Lakhin writes: > Shouldn't pg_ls_dir_files() retry stat() on ERROR_ACCESS_DENIED just > like the pgwin32_open() does to ignore files in "delete pending" state? That would soon lead us to changing every stat() caller in the system to have Windows-specific looping logic. No thanks. If

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-11-13 Thread Alexander Lakhin
Hello hackers, 31.03.2020 19:41, Tom Lane wrote: > Justin Pryzby writes: >> I suggest to leave stat() alone in your patch for stable releases. I think >> it's okay if we change behavior so that a broken symlink is skipped instead >> of >> erroring (as a side effect of skipping ENOENT with

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-31 Thread Tom Lane
Justin Pryzby writes: > I suggest to leave stat() alone in your patch for stable releases. I think > it's okay if we change behavior so that a broken symlink is skipped instead of > erroring (as a side effect of skipping ENOENT with stat()). But not okay if > we > change pg_ls_logdir() to hide

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-31 Thread Justin Pryzby
On Tue, Mar 31, 2020 at 07:36:03AM +0200, Fabien COELHO wrote: > > > As I wrote about an earlier version of the patch, ISTM that instead of > > > reinventing, extending, adapting various ls variants (with/without > > > metadata, which show only files, which shows target of links, which shows > > >

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-30 Thread Fabien COELHO
Hello, As I wrote about an earlier version of the patch, ISTM that instead of reinventing, extending, adapting various ls variants (with/without metadata, which show only files, which shows target of links, which shows directory, etc.) we would just need *one* postgres "ls" implementation

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-30 Thread Tom Lane
Fabien COELHO writes: > As I wrote about an earlier version of the patch, ISTM that instead of > reinventing, extending, adapting various ls variants (with/without > metadata, which show only files, which shows target of links, which shows > directory, etc.) we would just need *one* postgres

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-29 Thread Fabien COELHO
Hello Justin, Well, the following comment says "ignore anything but regular files", so I'm supposing that that is the behavior that we actually want here and failed to implement correctly. There might be scope for additional directory-reading functions, but I'd think you'd want more

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-29 Thread Justin Pryzby
On Sun, Mar 29, 2020 at 01:22:04PM -0400, Tom Lane wrote: > Justin Pryzby writes: > > On Sun, Mar 29, 2020 at 12:37:05PM -0400, Tom Lane wrote: > >> After looking at the callers of pg_ls_dir_files, and noticing that > >> it's already defined to ignore anything that's not a regular file, > >> I

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-29 Thread Tom Lane
Justin Pryzby writes: > On Sun, Mar 29, 2020 at 12:37:05PM -0400, Tom Lane wrote: >> After looking at the callers of pg_ls_dir_files, and noticing that >> it's already defined to ignore anything that's not a regular file, >> I think switching to lstat makes sense. > Yea, only pg_ls_dir() shows

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-29 Thread Justin Pryzby
On Sun, Mar 29, 2020 at 12:37:05PM -0400, Tom Lane wrote: > I wrote: > > Justin Pryzby writes: > >> Maybe we should lstat() the file to determine if it's a dangling link; if > >> lstat() fails, then skip it. Currently, we use stat(), which shows > >> metdata of > >> a link's *target*. Maybe

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-29 Thread Tom Lane
I wrote: > Justin Pryzby writes: >> Maybe we should lstat() the file to determine if it's a dangling link; if >> lstat() fails, then skip it. Currently, we use stat(), which shows metdata >> of >> a link's *target*. Maybe we'd change that. > Hm, good point that ENOENT could refer to a

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-28 Thread Tom Lane
Justin Pryzby writes: > On Sat, Mar 28, 2020 at 01:13:54PM -0400, Tom Lane wrote: >> so I propose that we fix these directory-scanning functions to silently >> ignore ENOENT failures from stat(). Are there any for which we should not do >> that? > Maybe we should lstat() the file to determine

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-28 Thread Justin Pryzby
On Sat, Mar 28, 2020 at 01:13:54PM -0400, Tom Lane wrote: > The buildfarm just showed up another instability in the test cases > we added: Yea, as you said, this is an issue with the *testcase*. The function behavior didn't change, we just weren't previously exercising it. > select (w).size =

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-28 Thread Tom Lane
Justin Pryzby writes: > Thanks for fixing my test case and pushing. The buildfarm just showed up another instability in the test cases we added: === regression.diffs diff -U3

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-16 Thread Justin Pryzby
On Mon, Mar 16, 2020 at 09:38:50PM -0400, Tom Lane wrote: > Justin Pryzby writes: > > v2 attached - I will add to next CF in case you want to defer it until > > later. > > Thanks, reviewed and pushed. Since this is a bug fix (at least in part) > I didn't want to wait. Thanks for fixing my

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-16 Thread Tom Lane
Justin Pryzby writes: > v2 attached - I will add to next CF in case you want to defer it until later. Thanks, reviewed and pushed. Since this is a bug fix (at least in part) I didn't want to wait. regards, tom lane

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-16 Thread Justin Pryzby
On Thu, Mar 12, 2020 at 07:11:56AM -0500, Justin Pryzby wrote: > > Do you want to have a go at that? > > First draft attached. Note that I handled pg_ls_dir, even though I'm > proposing > on the other thread to collapse/merge/meld it with pg_ls_dir_files [0]. > Possibly that's a bad idea with

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-12 Thread Tom Lane
Alvaro Herrera writes: > I wonder if this isn't saying that the whole value-per-call protocol is > bogus, in that it seems impossible to write a useful function with it. Only if you have a *very* narrow definition of "useful function". If you look through SRF_RETURN_DONE callers, only a small

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-12 Thread Alvaro Herrera
Nitpick: please see c4dcd9144ba6. > From: Justin Pryzby > Date: Wed, 11 Mar 2020 10:09:18 -0500 > Subject: [PATCH] SRF: avoid leaking resources if not run to completion > > Change to return a tuplestore populated immediately and returned in full. > > Discussion: >

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-12 Thread Justin Pryzby
On Wed, Mar 11, 2020 at 03:32:38PM -0400, Tom Lane wrote: > > I patched this one to see what it looks like and to allow /hopefully/ moving > > forward one way or another with the pg_ls_tmpfile() patch set (or at least > > avoid trying to do anything there which is too inconsistent with this fix).

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-12 Thread Justin Pryzby
On Sun, Mar 08, 2020 at 04:30:44PM -0400, Tom Lane wrote: > BTW, another thing I noticed while looking around is that some of > the functions using SRF_RETURN_DONE() think they should clean up > memory beforehand. This is a waste of code/cycles, as long as the > memory was properly allocated in

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-11 Thread Tom Lane
Justin Pryzby writes: >>> On Sun, Mar 08, 2020 at 02:37:49PM -0400, Tom Lane wrote: I guess we ought to change that function to use returns-a-tuplestore protocol instead of thinking it can hold a directory open across calls. > I patched this one to see what it looks like and to allow

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-11 Thread Justin Pryzby
On Sun, Mar 08, 2020 at 03:40:09PM -0400, Tom Lane wrote: > Justin Pryzby writes: > > On Sun, Mar 08, 2020 at 02:37:49PM -0400, Tom Lane wrote: > >> I guess we ought to change that function to use returns-a-tuplestore > >> protocol instead of thinking it can hold a directory open across calls. >

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-08 Thread Tom Lane
I wrote: > I've just finished scanning the source code and concluding that all > of these functions are similarly broken: > pg_ls_dir > pg_ls_dir_files > pg_tablespace_databases > pg_logdir_ls_internal > pg_timezone_names > pgrowlocks BTW, another thing I noticed while looking around is that some

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-08 Thread Tom Lane
Justin Pryzby writes: > On Sun, Mar 08, 2020 at 02:37:49PM -0400, Tom Lane wrote: >> I guess we ought to change that function to use returns-a-tuplestore >> protocol instead of thinking it can hold a directory open across calls. >> It's not hard to think of use-cases where the existing behavior

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-08 Thread Justin Pryzby
On Sun, Mar 08, 2020 at 02:37:49PM -0400, Tom Lane wrote: > Justin Pryzby writes: > > While working on a patch, I noticed this pre-existing behavior, which seems > > to > > be new since v11, maybe due to changes to SRF. > > > |postgres=# SELECT pg_ls_dir('.') LIMIT 1; > > |WARNING: 1 temporary

Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-08 Thread Tom Lane
Justin Pryzby writes: > While working on a patch, I noticed this pre-existing behavior, which seems to > be new since v11, maybe due to changes to SRF. > |postgres=# SELECT pg_ls_dir('.') LIMIT 1; > |WARNING: 1 temporary files and directories not closed at end-of-transaction Hmm, actually it

pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction

2020-03-08 Thread Justin Pryzby
While working on a patch, I noticed this pre-existing behavior, which seems to be new since v11, maybe due to changes to SRF. |postgres=# SELECT pg_ls_dir('.') LIMIT 1; |WARNING: 1 temporary files and directories not closed at end-of-transaction |pg_ls_dir | pg_dynshmem |postgres=# SELECT