Avoid unnecessary table open/close for TRUNCATE foo, foo, foo; kind of commands
Hi,
While checking the ExecuteTruncate code for the FOREIGN TRUNCATE
feature, I saw that we filter out the duplicate relations specified in
the TRUNCATE command. But before skipping the duplicates, we are just
opening the relation, then if it is present in the already seen
relids, then closing it and continuing further.
I think we can just have the duplicate checking before table_open so
that in cases like TRUNCATE foo, foo, foo, foo; we could save costs of
table_open and table_close. Attaching a small patch. Thoughts?
This is just like what we already do for child tables, see following
in ExecuteTruncate:
foreach(child, children)
{
Oid childrelid = lfirst_oid(child);
if (list_member_oid(relids, childrelid))
continue;
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-0001-Avoid-unnecessary-table-open-close-for-TRUNCATE-f.patchapplication/octet-stream; name=v1-0001-Avoid-unnecessary-table-open-close-for-TRUNCATE-f.patchDownload
From 48397388e38bf912a50f5e2bdcb34237a3c097f7 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Fri, 9 Apr 2021 20:24:37 +0530
Subject: [PATCH v1] Avoid unnecessary table open/close for TRUNCATE foo, foo,
foo; kind of commands
In ExecuteTruncate, we filter out the duplicate relations specified
in the TRUNCATE command. But before skipping the duplicates, we
just open the relation, then if it is present in the already seen
relids, then close the relation and continue further.
We can just have the duplicate checking before table_open so that
in the cases like TRUNCATE foo, foo, foo, foo; we could save costs
of table_open and table_close for the relation foo.
---
src/backend/commands/tablecmds.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1f19629a94..5a7d6f3954 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1638,15 +1638,12 @@ ExecuteTruncate(TruncateStmt *stmt)
0, RangeVarCallbackForTruncate,
NULL);
- /* open the relation, we already hold a lock on it */
- rel = table_open(myrelid, NoLock);
-
/* don't throw error for "TRUNCATE foo, foo" */
if (list_member_oid(relids, myrelid))
- {
- table_close(rel, lockmode);
continue;
- }
+
+ /* open the relation, we already hold a lock on it */
+ rel = table_open(myrelid, NoLock);
/*
* RangeVarGetRelidExtended() has done most checks with its callback,
--
2.25.1
On Fri, Apr 9, 2021 at 8:51 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Hi,
While checking the ExecuteTruncate code for the FOREIGN TRUNCATE
feature, I saw that we filter out the duplicate relations specified in
the TRUNCATE command. But before skipping the duplicates, we are just
opening the relation, then if it is present in the already seen
relids, then closing it and continuing further.I think we can just have the duplicate checking before table_open so
that in cases like TRUNCATE foo, foo, foo, foo; we could save costs of
table_open and table_close. Attaching a small patch. Thoughts?This is just like what we already do for child tables, see following
in ExecuteTruncate:
foreach(child, children)
{
Oid childrelid = lfirst_oid(child);if (list_member_oid(relids, childrelid))
continue;
Well yes, the patch looks pretty much reasonable to be.
Regards,
Amul
On 2021/04/10 0:39, Amul Sul wrote:
On Fri, Apr 9, 2021 at 8:51 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Hi,
While checking the ExecuteTruncate code for the FOREIGN TRUNCATE
feature, I saw that we filter out the duplicate relations specified in
the TRUNCATE command. But before skipping the duplicates, we are just
opening the relation, then if it is present in the already seen
relids, then closing it and continuing further.I think we can just have the duplicate checking before table_open so
that in cases like TRUNCATE foo, foo, foo, foo; we could save costs of
table_open and table_close. Attaching a small patch. Thoughts?This is just like what we already do for child tables, see following
in ExecuteTruncate:
foreach(child, children)
{
Oid childrelid = lfirst_oid(child);if (list_member_oid(relids, childrelid))
continue;Well yes, the patch looks pretty much reasonable to be.
LGTM, too. I will commit this patch.
Though that code exists even in older version, I'm not thinking
to back-patch that because it's not a bug.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Fri, Apr 9, 2021 at 9:23 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/04/10 0:39, Amul Sul wrote:
On Fri, Apr 9, 2021 at 8:51 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Hi,
While checking the ExecuteTruncate code for the FOREIGN TRUNCATE
feature, I saw that we filter out the duplicate relations specified in
the TRUNCATE command. But before skipping the duplicates, we are just
opening the relation, then if it is present in the already seen
relids, then closing it and continuing further.I think we can just have the duplicate checking before table_open so
that in cases like TRUNCATE foo, foo, foo, foo; we could save costs of
table_open and table_close. Attaching a small patch. Thoughts?This is just like what we already do for child tables, see following
in ExecuteTruncate:
foreach(child, children)
{
Oid childrelid = lfirst_oid(child);if (list_member_oid(relids, childrelid))
continue;Well yes, the patch looks pretty much reasonable to be.
LGTM, too. I will commit this patch.
Though that code exists even in older version, I'm not thinking
to back-patch that because it's not a bug.
Agree, thanks Fujii-San.
Regards,
Amul
On Fri, Apr 9, 2021 at 9:23 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/04/10 0:39, Amul Sul wrote:
On Fri, Apr 9, 2021 at 8:51 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Hi,
While checking the ExecuteTruncate code for the FOREIGN TRUNCATE
feature, I saw that we filter out the duplicate relations specified in
the TRUNCATE command. But before skipping the duplicates, we are just
opening the relation, then if it is present in the already seen
relids, then closing it and continuing further.I think we can just have the duplicate checking before table_open so
that in cases like TRUNCATE foo, foo, foo, foo; we could save costs of
table_open and table_close. Attaching a small patch. Thoughts?This is just like what we already do for child tables, see following
in ExecuteTruncate:
foreach(child, children)
{
Oid childrelid = lfirst_oid(child);if (list_member_oid(relids, childrelid))
continue;Well yes, the patch looks pretty much reasonable to be.
LGTM, too. I will commit this patch.
Though that code exists even in older version, I'm not thinking
to back-patch that because it's not a bug.
Thanks. +1 to not back-patch.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On 2021/04/10 11:32, Bharath Rupireddy wrote:
On Fri, Apr 9, 2021 at 9:23 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/04/10 0:39, Amul Sul wrote:
On Fri, Apr 9, 2021 at 8:51 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Hi,
While checking the ExecuteTruncate code for the FOREIGN TRUNCATE
feature, I saw that we filter out the duplicate relations specified in
the TRUNCATE command. But before skipping the duplicates, we are just
opening the relation, then if it is present in the already seen
relids, then closing it and continuing further.I think we can just have the duplicate checking before table_open so
that in cases like TRUNCATE foo, foo, foo, foo; we could save costs of
table_open and table_close. Attaching a small patch. Thoughts?This is just like what we already do for child tables, see following
in ExecuteTruncate:
foreach(child, children)
{
Oid childrelid = lfirst_oid(child);if (list_member_oid(relids, childrelid))
continue;Well yes, the patch looks pretty much reasonable to be.
LGTM, too. I will commit this patch.
Though that code exists even in older version, I'm not thinking
to back-patch that because it's not a bug.Thanks. +1 to not back-patch.
Pushed only to the master. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION