public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v28 02/11] Add tests on pg_ls_dir before changing it
5+ messages / 3 participants
[nested] [flat]
* [PATCH v28 02/11] Add tests on pg_ls_dir before changing it
@ 2020-03-17 18:16 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Justin Pryzby @ 2020-03-17 18:16 UTC (permalink / raw)
---
src/test/regress/expected/misc_functions.out | 24 ++++++++++++++++++++
src/test/regress/sql/misc_functions.sql | 8 +++++++
2 files changed, 32 insertions(+)
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index e845042d38..ea0fc48dbd 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -214,6 +214,30 @@ select count(*) > 0 from
t
(1 row)
+select * from (select pg_ls_dir('.', false, true) as name) as ls where ls.name='.'; -- include_dot_dirs=true
+ name
+------
+ .
+(1 row)
+
+select * from (select pg_ls_dir('.', false, false) as name) as ls where ls.name='.'; -- include_dot_dirs=false
+ name
+------
+(0 rows)
+
+select pg_ls_dir('does not exist', true, false); -- ok with missingok=true
+ pg_ls_dir
+-----------
+(0 rows)
+
+select pg_ls_dir('does not exist'); -- fails with missingok=false
+ERROR: could not open directory "does not exist": No such file or directory
+-- Check that expected columns are present
+select * from pg_stat_file('.') limit 0;
+ size | access | modification | change | creation | isdir
+------+--------+--------------+--------+----------+-------
+(0 rows)
+
--
-- Test adding a support function to a subject function
--
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index a398349afc..eb6ac12ab4 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -69,6 +69,14 @@ select count(*) > 0 from
where spcname = 'pg_default') pts
join pg_database db on pts.pts = db.oid;
+select * from (select pg_ls_dir('.', false, true) as name) as ls where ls.name='.'; -- include_dot_dirs=true
+select * from (select pg_ls_dir('.', false, false) as name) as ls where ls.name='.'; -- include_dot_dirs=false
+select pg_ls_dir('does not exist', true, false); -- ok with missingok=true
+select pg_ls_dir('does not exist'); -- fails with missingok=false
+
+-- Check that expected columns are present
+select * from pg_stat_file('.') limit 0;
+
--
-- Test adding a support function to a subject function
--
--
2.17.0
--vk/v8fjDPiDepTtA
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v28-0003-Add-pg_ls_dir_metadata-to-list-a-dir-with-file-m.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Improve the efficiency of _bt_killitems.
@ 2024-11-01 07:19 feichanghong <[email protected]>
2024-11-01 08:24 ` Re: Improve the efficiency of _bt_killitems. Heikki Linnakangas <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: feichanghong @ 2024-11-01 07:19 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]
Hi hackers,
In the _bt_killitems function, the following logic is present: we search to the
right for an index item that matches the heap TID and attempt to mark it as
dead. If that index item has already been marked as dead by other concurrent
processes, we will continue searching. However, there should not be any more
matching index items on the current page.
```
while (offnum <= maxoff)
{
...
/*
* Mark index item as dead, if it isn't already. Since this
* happens while holding a buffer lock possibly in shared mode,
* it's possible that multiple processes attempt to do this
* simultaneously, leading to multiple full-page images being sent
* to WAL (if wal_log_hints or data checksums are enabled), which
* is undesirable.
*/
if (killtuple && !ItemIdIsDead(iid))
{
/* found the item/all posting list items */
ItemIdMarkDead(iid);
killedsomething = true;
break; /* out of inner search loop */
}
offnum = OffsetNumberNext(offnum);
}
```
Perhaps we should exit the current loop immediately when the killtuple is set,
stopping the search to the right. This could improve efficiency, especially
when the index item to be killed is the first one on the current page:
```
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index b4ba51357a5..529a3083165 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -4298,11 +4298,14 @@ _bt_killitems(IndexScanDesc scan)
* to WAL (if wal_log_hints or data checksums are enabled), which
* is undesirable.
*/
- if (killtuple && !ItemIdIsDead(iid))
+ if (killtuple)
{
- /* found the item/all posting list items */
- ItemIdMarkDead(iid);
- killedsomething = true;
+ if (!ItemIdIsDead(iid))
+ {
+ /* found the item/all posting list items */
+ ItemIdMarkDead(iid);
+ killedsomething = true;
+ }
break; /* out of inner search loop */
}
offnum = OffsetNumberNext(offnum);
```
Best Regards,
Fei Changhong
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Improve the efficiency of _bt_killitems.
2024-11-01 07:19 Improve the efficiency of _bt_killitems. feichanghong <[email protected]>
@ 2024-11-01 08:24 ` Heikki Linnakangas <[email protected]>
2024-11-01 08:41 ` Re: Improve the efficiency of _bt_killitems. feichanghong <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Heikki Linnakangas @ 2024-11-01 08:24 UTC (permalink / raw)
To: feichanghong <[email protected]>; [email protected]; +Cc: [email protected]
On 01/11/2024 09:19, feichanghong wrote:
> Hi hackers,
>
> In the _bt_killitems function, the following logic is present: we
> search to the right for an index item that matches the heap TID and
> attempt to mark it as dead. If that index item has already been
> marked as dead by other concurrent processes, we will continue
> searching. However, there should not be any more matching index items
> on the current page.
Why could there not be more matching items on the page?
Are you assuming a unique index? Even then it's not right; you can have
multiple index entries point to different heap tuples with the same key,
as long as they're not visible at the same time. For example, if you
UPDATE or DELETE+INSERT a row.
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Improve the efficiency of _bt_killitems.
2024-11-01 07:19 Improve the efficiency of _bt_killitems. feichanghong <[email protected]>
2024-11-01 08:24 ` Re: Improve the efficiency of _bt_killitems. Heikki Linnakangas <[email protected]>
@ 2024-11-01 08:41 ` feichanghong <[email protected]>
2024-11-01 10:50 ` Re: Improve the efficiency of _bt_killitems. Heikki Linnakangas <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: feichanghong @ 2024-11-01 08:41 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; +Cc: [email protected]; [email protected]
> On Nov 1, 2024, at 16:24, Heikki Linnakangas <[email protected]> wrote:
>
> On 01/11/2024 09:19, feichanghong wrote:
>> Hi hackers,
>> In the _bt_killitems function, the following logic is present: we search to the right for an index item that matches the heap TID and attempt to mark it as dead. If that index item has already been marked as dead by other concurrent processes, we will continue
>> searching. However, there should not be any more matching index items
>> on the current page.
> Why could there not be more matching items on the page?
>
> Are you assuming a unique index? Even then it's not right; you can have multiple index entries point to different heap tuples with the same key, as long as they're not visible at the same time. For example, if you UPDATE or DELETE+INSERT a row.
Maybe I didn't describe it clearly. What I meant to say is that there shouldn't
be multiple index items on the current page pointing to the same heap TID(ctid).
rather than the same index key.
Best Regards,
Fei Changhong
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Improve the efficiency of _bt_killitems.
2024-11-01 07:19 Improve the efficiency of _bt_killitems. feichanghong <[email protected]>
2024-11-01 08:24 ` Re: Improve the efficiency of _bt_killitems. Heikki Linnakangas <[email protected]>
2024-11-01 08:41 ` Re: Improve the efficiency of _bt_killitems. feichanghong <[email protected]>
@ 2024-11-01 10:50 ` Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Heikki Linnakangas @ 2024-11-01 10:50 UTC (permalink / raw)
To: feichanghong <[email protected]>; +Cc: [email protected]; [email protected]
On 01/11/2024 10:41, feichanghong wrote:
>
>
>> On Nov 1, 2024, at 16:24, Heikki Linnakangas <[email protected]> wrote:
>>
>> On 01/11/2024 09:19, feichanghong wrote:
>>> Hi hackers,
>>> In the _bt_killitems function, the following logic is present: we
>>> search to the right for an index item that matches the heap TID and
>>> attempt to mark it as dead. If that index item has already been
>>> marked as dead by other concurrent processes, we will continue
>>> searching. However, there should not be any more matching index items
>>> on the current page.
>> Why could there not be more matching items on the page?
>>
>> Are you assuming a unique index? Even then it's not right; you can
>> have multiple index entries point to different heap tuples with the
>> same key, as long as they're not visible at the same time. For
>> example, if you UPDATE or DELETE+INSERT a row.
>
> Maybe I didn't describe it clearly. What I meant to say is that there
> shouldn't
> be multiple index items on the current page pointing to the same heap
> TID(ctid).
> rather than the same index key.
Ah, gotcha. That makes sense.
There's more we could do here in the similar vein:
If the TID was found in a posting list, but not all of the items were
dead, 'killtuple' is set to false so we will still continue the search.
Then there's this comment:
> /*
> * Don't bother advancing the outermost loop's int iterator to
> * avoid processing killed items that relate to the same
> * offnum/posting list tuple. This micro-optimization hardly
> * seems worth it. (Further iterations of the outermost loop
> * will fail to match on this same posting list's first heap
> * TID instead, so we'll advance to the next offnum/index
> * tuple pretty quickly.)
> */
I think we should do that micro-optimization. It's a single line of code
and a single instruction to advance 'i' variable AFAICS. While I agree
with the comment that it doesn't matter much performance-wise, it seems
simpler to just do it than explain why we don't do it.
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-11-01 10:50 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-17 18:16 [PATCH v28 02/11] Add tests on pg_ls_dir before changing it Justin Pryzby <[email protected]>
2024-11-01 07:19 Improve the efficiency of _bt_killitems. feichanghong <[email protected]>
2024-11-01 08:24 ` Re: Improve the efficiency of _bt_killitems. Heikki Linnakangas <[email protected]>
2024-11-01 08:41 ` Re: Improve the efficiency of _bt_killitems. feichanghong <[email protected]>
2024-11-01 10:50 ` Re: Improve the efficiency of _bt_killitems. Heikki Linnakangas <[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