[PATCH] Improve function toast_delete_external (src/backend/access/table/toast_helper.c)
Hi,
I think this change can improve this particular function by avoiding
touching value if not needed.
Test if not isnull is cheaper than test TupleDescAttr is -1.
best regards,
Ranier Vilela
Attachments:
v1_tiny_improvemnt_toast_helper.patchapplication/octet-stream; name=v1_tiny_improvemnt_toast_helper.patchDownload
diff --git a/src/backend/access/table/toast_helper.c b/src/backend/access/table/toast_helper.c
index 0cc5a30f9b..0fee691a8b 100644
--- a/src/backend/access/table/toast_helper.c
+++ b/src/backend/access/table/toast_helper.c
@@ -324,13 +324,11 @@ toast_delete_external(Relation rel, Datum *values, bool *isnull,
for (i = 0; i < numAttrs; i++)
{
- if (TupleDescAttr(tupleDesc, i)->attlen == -1)
+ if (!isnull[i] && TupleDescAttr(tupleDesc, i)->attlen == -1)
{
Datum value = values[i];
- if (isnull[i])
- continue;
- else if (VARATT_IS_EXTERNAL_ONDISK(PointerGetDatum(value)))
+ if (VARATT_IS_EXTERNAL_ONDISK(PointerGetDatum(value)))
toast_delete_datum(rel, value, is_speculative);
}
}
Hi,
On Wed, Feb 09, 2022 at 07:56:35AM -0300, Ranier Vilela wrote:
I think this change can improve this particular function by avoiding
touching value if not needed.
Test if not isnull is cheaper than test TupleDescAttr is -1.
It looks sensible.
Hi,
I've applied your patch. I think it's reasonable.
but IMHO It looks more complicated to read because of many conditions
in if statement.
so what about just moving up if-statement?
Thanks.
Dong Wook Lee
2022년 2월 9일 (수) 오후 7:56, Ranier Vilela <ranier.vf@gmail.com>님이 작성:
Show quoted text
Hi,
I think this change can improve this particular function by avoiding touching value if not needed.
Test if not isnull is cheaper than test TupleDescAttr is -1.best regards,
Ranier Vilela
Attachments:
v2_tiny_improvemnt_toast_helper.patchapplication/octet-stream; name=v2_tiny_improvemnt_toast_helper.patchDownload
diff --git a/src/backend/access/table/toast_helper.c b/src/backend/access/table/toast_helper.c
index 0cc5a30f9b..c04f9c14d2 100644
--- a/src/backend/access/table/toast_helper.c
+++ b/src/backend/access/table/toast_helper.c
@@ -326,10 +326,11 @@ toast_delete_external(Relation rel, Datum *values, bool *isnull,
{
if (TupleDescAttr(tupleDesc, i)->attlen == -1)
{
- Datum value = values[i];
-
if (isnull[i])
continue;
+
+ Datum value = values[i];
+
else if (VARATT_IS_EXTERNAL_ONDISK(PointerGetDatum(value)))
toast_delete_datum(rel, value, is_speculative);
}
Oops, I realized that I made a mistake in my patch and now I have corrected it.
2022년 2월 9일 (수) 오후 11:33, Dong Wook Lee <sh95119@gmail.com>님이 작성:
Show quoted text
Hi,
I've applied your patch. I think it's reasonable.
but IMHO It looks more complicated to read because of many conditions
in if statement.
so what about just moving up if-statement?Thanks.
Dong Wook Lee2022년 2월 9일 (수) 오후 7:56, Ranier Vilela <ranier.vf@gmail.com>님이 작성:
Hi,
I think this change can improve this particular function by avoiding touching value if not needed.
Test if not isnull is cheaper than test TupleDescAttr is -1.best regards,
Ranier Vilela
Attachments:
v3_tiny_improvemnt_toast_helper.patchapplication/octet-stream; name=v3_tiny_improvemnt_toast_helper.patchDownload
diff --git a/src/backend/access/table/toast_helper.c b/src/backend/access/table/toast_helper.c
index 0cc5a30f9b..643a3b05ad 100644
--- a/src/backend/access/table/toast_helper.c
+++ b/src/backend/access/table/toast_helper.c
@@ -326,11 +326,12 @@ toast_delete_external(Relation rel, Datum *values, bool *isnull,
{
if (TupleDescAttr(tupleDesc, i)->attlen == -1)
{
- Datum value = values[i];
-
if (isnull[i])
continue;
- else if (VARATT_IS_EXTERNAL_ONDISK(PointerGetDatum(value)))
+
+ Datum value = values[i];
+
+ if (VARATT_IS_EXTERNAL_ONDISK(PointerGetDatum(value)))
toast_delete_datum(rel, value, is_speculative);
}
}
Our style is that we group all declarations in a block to appear at the
top. We don't mix declarations and statements.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
Yes, now I understand it.
Thank you for letting me know about that.
Thanks.
2022년 2월 10일 (목) 00:39, Ranier Vilela <ranier.vf@gmail.com>님이 작성:
Show quoted text
Em qua., 9 de fev. de 2022 às 11:34, Dong Wook Lee <sh95119@gmail.com>
escreveu:Hi,
I've applied your patch. I think it's reasonable.
but IMHO It looks more complicated to read because of many conditions
in if statement.
so what about just moving up if-statement?No.
Your version is worse than HEAD, please don't.1. Do not declare variables after statement.
Although Postgres accepts C99, this is not acceptable, for a number of
problems, already discussed.2. Still slow unnecessarily, because still test TupleDescAttr(tupleDesc,
i)->attlen,
which is much more onerous than test isnull.3. We don't write Baby C code.
C has short break expressions, use-it!My version is the right thing, here.
regards,
Ranier Vilela
Import Notes
Reply to msg id not found: CAEudQApNmzLc7xg68WzNfsW_xPiFBVKMzsRF25Rm1G6mMYedA@mail.gmail.com