Обсуждение: Unify drop-by-OID functions

Поиск
Список
Период
Сортировка

Unify drop-by-OID functions

От
Peter Eisentraut
Дата:
[proposal for PG 14]

There are a number of Remove${Something}ById() functions that are 
essentially identical in structure and only different in which catalog 
they are working on.  This patch refactors this to be one generic 
function.  The information about which oid column, index, etc. to use 
was already available in ObjectProperty for most catalogs, in a few 
cases it was easily added.

Conceivably, this could be taken further by categorizing more special 
cases as ObjectProperty fields or something like that, but this seemed 
like a good balance.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения

Re: Unify drop-by-OID functions

От
Pavel Stehule
Дата:


pá 1. 5. 2020 v 16:39 odesílatel Peter Eisentraut <peter.eisentraut@2ndquadrant.com> napsal:
[proposal for PG 14]

There are a number of Remove${Something}ById() functions that are
essentially identical in structure and only different in which catalog
they are working on.  This patch refactors this to be one generic
function.  The information about which oid column, index, etc. to use
was already available in ObjectProperty for most catalogs, in a few
cases it was easily added.

Conceivably, this could be taken further by categorizing more special
cases as ObjectProperty fields or something like that, but this seemed
like a good balance.

+1

nice

Pavel


--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Unify drop-by-OID functions

От
Robert Haas
Дата:
On Fri, May 1, 2020 at 10:51 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> +1

+1 from me, too, but I have a few suggestions:

+DropGenericById(const ObjectAddress *object)

How about "Generic" -> "Object" or "Generic" -> "ObjectAddress"?

+ elog(ERROR, "cache lookup failed for %s entry %u",
+ elog(ERROR, "could not find tuple for class %u entry %u",

How about "entry" -> "with OID"?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Unify drop-by-OID functions

От
Ranier Vilela
Дата:
Em sex., 1 de mai. de 2020 às 11:39, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> escreveu:
[proposal for PG 14]

There are a number of Remove${Something}ById() functions that are
essentially identical in structure and only different in which catalog
they are working on.  This patch refactors this to be one generic
function.  The information about which oid column, index, etc. to use
was already available in ObjectProperty for most catalogs, in a few
cases it was easily added.

Conceivably, this could be taken further by categorizing more special
cases as ObjectProperty fields or something like that, but this seemed
like a good balance.
Very good.

I can suggest improvements?

1. In case Object is cached, delay open_table until the last moment, for the row to be blocked as little as possible and close the table as quickly as possible.
2. In case Object is cached and the tuple is invalid, do not open table.
3. Otherwise, is it possible to call systable_endscan, after table_close?

I think that lock resources, for as little time as possible, it is an advantage..

+static void
+DropGenericById(const ObjectAddress *object)
+{
+ int cacheId;
+ Relation rel;
+ HeapTuple tup;
+
+ cacheId = get_object_catcache_oid(object->classId);
+
+ /*
+ * Use the system cache for the oid column, if one exists.
+ */
+ if (cacheId >= 0)
+ {
+ tup = SearchSysCache1(cacheId, ObjectIdGetDatum(object->objectId));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for %s entry %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+        rel = table_open(object->classId, RowExclusiveLock);
+ CatalogTupleDelete(rel, &tup->t_self);
+        table_close(rel, RowExclusiveLock);
+
+ ReleaseSysCache(tup);
+ }
+ else
+ {
+ ScanKeyData skey[1];
+ SysScanDesc scan;
+
+ ScanKeyInit(&skey[0],
+ get_object_attnum_oid(object->classId),
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(object->objectId));
+
+        rel = table_open(object->classId, RowExclusiveLock);
+ scan = systable_beginscan(rel, get_object_oid_index(object->classId), true,
+  NULL, 1, skey);
+
+ /* we expect exactly one match */
+ tup = systable_getnext(scan);
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "could not find tuple for class %u entry %u",
+ object->classId, object->objectId);
+
+ CatalogTupleDelete(rel, &tup->t_self);
+ systable_endscan(scan);
+        table_close(rel, RowExclusiveLock);
+ }
+}
+

regards,
Ranier Vilela

Re: Unify drop-by-OID functions

От
Peter Eisentraut
Дата:
On 2020-05-01 23:31, Ranier Vilela wrote:
> I can suggest improvements?
> 
> 1. In case Object is cached, delay open_table until the last moment, for 
> the row to be blocked as little as possible and close the table as 
> quickly as possible.
> 2. In case Object is cached and the tuple is invalid, do not open table.
> 3. Otherwise, is it possible to call systable_endscan, after table_close?

What do you mean by "object is cached"?

In any case, this is a refactoring patch, so significant changes to the 
internal logic would not really be in scope.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Unify drop-by-OID functions

От
Ranier Vilela
Дата:
Em sáb., 2 de mai. de 2020 às 05:01, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> escreveu:
On 2020-05-01 23:31, Ranier Vilela wrote:
> I can suggest improvements?
>
> 1. In case Object is cached, delay open_table until the last moment, for
> the row to be blocked as little as possible and close the table as
> quickly as possible.
> 2. In case Object is cached and the tuple is invalid, do not open table.
> 3. Otherwise, is it possible to call systable_endscan, after table_close?

What do you mean by "object is cached"?
Well, that's what I deduced from the cacheId variable name.

regards,
Ranier Vilela

Re: Unify drop-by-OID functions

От
Peter Eisentraut
Дата:
On 2020-05-01 17:44, Robert Haas wrote:
> On Fri, May 1, 2020 at 10:51 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> +1
> 
> +1 from me, too, but I have a few suggestions:
> 
> +DropGenericById(const ObjectAddress *object)
> 
> How about "Generic" -> "Object" or "Generic" -> "ObjectAddress"?

Changed to "Object", that also matches existing functions that operate 
on an ObjectAddress.

> + elog(ERROR, "cache lookup failed for %s entry %u",
> + elog(ERROR, "could not find tuple for class %u entry %u",
> 
> How about "entry" -> "with OID"?

I changed these to just

"cache lookup failed for %s %u"
"could not find tuple for %s %u"

which matches the existing wording for the not-refactored cases.  I 
don't recall why I went and reworded them.

New patch attached.  I'll park it until PG14 opens.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения

Re: Unify drop-by-OID functions

От
Robert Haas
Дата:
On Fri, May 1, 2020 at 5:32 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> I can suggest improvements?
>
> 1. In case Object is cached, delay open_table until the last moment, for the row to be blocked as little as possible
andclose the table as quickly as possible.
 
> 2. In case Object is cached and the tuple is invalid, do not open table.
> 3. Otherwise, is it possible to call systable_endscan, after table_close?
>
> I think that lock resources, for as little time as possible, it is an advantage..

Only if it's correct, which (3) definitely wouldn't be, and I'm
doubtful about (1) as well.

This reminds me: I think that the issues in
http://postgr.es/m/CA+TgmoYaFYRRdRZ94p_Qdt+1oONg6sMOvbkGHKVsFtONCrFkhw@mail.gmail.com
should be considered here - we should guarantee that there's a
snapshot registered continuously from before the call to
SearchSysCache1 until after the call to CatalogTupleDelete. In the
systable_beginscan case, we should be fine as long as the
systable_endscan follows the CatalogTupleDelete call.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Unify drop-by-OID functions

От
Ranier Vilela
Дата:
Em ter., 5 de mai. de 2020 às 13:06, Robert Haas <robertmhaas@gmail.com> escreveu:
On Fri, May 1, 2020 at 5:32 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> I can suggest improvements?
>
> 1. In case Object is cached, delay open_table until the last moment, for the row to be blocked as little as possible and close the table as quickly as possible.
> 2. In case Object is cached and the tuple is invalid, do not open table.
> 3. Otherwise, is it possible to call systable_endscan, after table_close?
>
> I think that lock resources, for as little time as possible, it is an advantage..

Only if it's correct, which (3) definitely wouldn't be, and I'm
doubtful about (1) as well.
Ok, so the question. If (3) is not safe, obvious we shouldn't use, and must call table_close, after systable_endscan.
Now (1) and (2), I would have no hesitation in using it.
I work with ERP, and throughout the time, the later, lock resources and release them soon, the better, for the performance of the system as a whole.
Even if it doesn't make much difference locally, using this process, throughout the system, efficiency is noticeable.
Apparently, it is more code, but it is less resources used and for less time.
And (2), if it is a case, frequently, no table would be blocked in this function.

Simple examples.

Exemple 1:
FILE * f;
f = fopen("data.txt", "r");
if (f != NULL) {
    char buf[512];
    size_t result;
    result = fread(&buf, sizeof(char), 512, f);
    fclose(f); // we no longer need the resource, release.
    if (result != 0) {
        process(buf);
        printf("buf=%s\n", buf);
    }
}

Exemple 2:
FILE * f;
f = fopen("data.txt", "r");
if (f != NULL) {
    char buf[512];
    size_t result;
    result = fread(&buf, sizeof(char), 512, f);
    if (result != 0) {
        process(buf);
        printf("buf=%s\n", buf);
    }
    fclose(f); // resource blocked until the end.
}

regards,
Ranier Vilela

Re: Unify drop-by-OID functions

От
Robert Haas
Дата:
On Tue, May 5, 2020 at 1:22 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> Ok, so the question. If (3) is not safe, obvious we shouldn't use, and must call table_close, after
systable_endscan.
> Now (1) and (2), I would have no hesitation in using it.
> I work with ERP, and throughout the time, the later, lock resources and release them soon, the better, for the
performanceof the system as a whole.
 
> Even if it doesn't make much difference locally, using this process, throughout the system, efficiency is
noticeable.
> Apparently, it is more code, but it is less resources used and for less time.
> And (2), if it is a case, frequently, no table would be blocked in this function.

Nobody here is going to question the concept that it's better to use
resources for less time rather than more, but the wisdom of sticking
to well-established coding patterns instead of inventing altogether
new ones is also well-understood. There are often good reasons why the
code is written in the way that it is, and it's important to
understand those before proposing to change things.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Unify drop-by-OID functions

От
Ranier Vilela
Дата:
Em ter., 5 de mai. de 2020 às 14:29, Robert Haas <robertmhaas@gmail.com> escreveu:
On Tue, May 5, 2020 at 1:22 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> Ok, so the question. If (3) is not safe, obvious we shouldn't use, and must call table_close, after systable_endscan.
> Now (1) and (2), I would have no hesitation in using it.
> I work with ERP, and throughout the time, the later, lock resources and release them soon, the better, for the performance of the system as a whole.
> Even if it doesn't make much difference locally, using this process, throughout the system, efficiency is noticeable.
> Apparently, it is more code, but it is less resources used and for less time.
> And (2), if it is a case, frequently, no table would be blocked in this function.

Nobody here is going to question the concept that it's better to use
resources for less time rather than more, but the wisdom of sticking
to well-established coding patterns instead of inventing altogether
new ones is also well-understood. There are often good reasons why the
code is written in the way that it is, and it's important to
understand those before proposing to change things.
I see, the famous "cliché".

regards,
Ranier Vilela

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Unify drop-by-OID functions

От
Robert Haas
Дата:
On Tue, May 5, 2020 at 1:43 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> I see, the famous "cliché".

By using the word cliché, and by putting it in quotes, you seem to
suggest that you consider my argument dubious. However, I stand by it.
Code shouldn't be changed without understanding the reasons behind the
current coding. Doing so very often breaks things.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Unify drop-by-OID functions

От
Ranier Vilela
Дата:
Em ter., 5 de mai. de 2020 às 14:57, Robert Haas <robertmhaas@gmail.com> escreveu:
On Tue, May 5, 2020 at 1:43 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> I see, the famous "cliché".

By using the word cliché, and by putting it in quotes, you seem to
suggest that you consider my argument dubious. However, I stand by it.
Code shouldn't be changed without understanding the reasons behind the
current coding. Doing so very often breaks things.
Sorry Robert, It was not my intention. I didn't know that using quotes would change your understanding.
Of course, I find your arguments very valid and valuable.
And I understand that there are many interrelated things, which can break if done in the wrong order.
Maybe I used the wrong word, in this case, the cliché.
What I mean is, the cliché, does some strange things, like leaving variables to be declared, assigned in and not used.
And in that specific case, leaving resources blocked, which perhaps, in my humble opinion, could be released quickly.
I think the expected behavior is being the same, with the changes I proposed, IMHO.

+static void
+DropObjectById(const ObjectAddress *object)
+{
+ int cacheId;
+ Relation rel;
+ HeapTuple tup;
+
+ cacheId = get_object_catcache_oid(object->classId);
+
+ /*
+ * Use the system cache for the oid column, if one exists.
+ */
+ if (cacheId >= 0)
+ {
+ tup = SearchSysCache1(cacheId, ObjectIdGetDatum(object->objectId));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+    rel = table_open(object->classId, RowExclusiveLock);
+ CatalogTupleDelete(rel, &tup->t_self);
+    table_close(rel, RowExclusiveLock);
+
+ ReleaseSysCache(tup);
+ }
+ else
+ {
+ ScanKeyData skey[1];
+ SysScanDesc scan;
+
+ ScanKeyInit(&skey[0],
+ get_object_attnum_oid(object->classId),
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(object->objectId));
+
+    rel = table_open(object->classId, RowExclusiveLock);
+ scan = systable_beginscan(rel, get_object_oid_index(object->classId), true,
+  NULL, 1, skey);
+
+ /* we expect exactly one match */
+ tup = systable_getnext(scan);
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "could not find tuple for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+ CatalogTupleDelete(rel, &tup->t_self);
+ systable_endscan(scan);
+
+    table_close(rel, RowExclusiveLock);
+ }
+}

And again, your opinion is very important to me.

best regards,
Ranier Vilela

Re: Unify drop-by-OID functions

От
Alvaro Herrera
Дата:
On 2020-May-05, Ranier Vilela wrote:

> And in that specific case, leaving resources blocked, which perhaps, in my
> humble opinion, could be released quickly.

I very much doubt that you can measure any difference at all between
these two codings of the function.

I agree with the principle you mention, of not holding resources for
long if they can be released earlier; but in this case the table_close
call occurs across a ReleaseSysCache() call, which is hardly of
significance.  It's not like you have to wait for some other
transaction, or wait for I/O, or anything like that that would make it
measurable.  

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Unify drop-by-OID functions

От
Ranier Vilela
Дата:
Em ter., 5 de mai. de 2020 às 18:11, Alvaro Herrera <alvherre@2ndquadrant.com> escreveu:
On 2020-May-05, Ranier Vilela wrote:

> And in that specific case, leaving resources blocked, which perhaps, in my
> humble opinion, could be released quickly.

I very much doubt that you can measure any difference at all between
these two codings of the function.

I agree with the principle you mention, of not holding resources for
long if they can be released earlier; but in this case the table_close
call occurs across a ReleaseSysCache() call, which is hardly of
significance.  It's not like you have to wait for some other
transaction, or wait for I/O, or anything like that that would make it
measurable. 
Locally it may not be as efficient, but it is a question, to follow the good programming practices, among them, not to break anything, not to block, and if it is not possible, release soon.
In at least one case, there will not even be a block, which is an improvement.

Another version, that could, be, without using ERROR.

+static void
+DropObjectById(const ObjectAddress *object)
+{
+ int cacheId;
+ Relation rel;
+ HeapTuple tup;
+
+ cacheId = get_object_catcache_oid(object->classId);
+
+ /*
+ * Use the system cache for the oid column, if one exists.
+ */
+ if (cacheId >= 0)
+ {
+ tup = SearchSysCache1(cacheId, ObjectIdGetDatum(object->objectId));
+ if (HeapTupleIsValid(tup)) {
+        rel = table_open(object->classId, RowExclusiveLock);
+    CatalogTupleDelete(rel, &tup->t_self);
+        table_close(rel, RowExclusiveLock);
+    ReleaseSysCache(tup);
+       }
+       else
+ elog(LOG, "cache lookup failed for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+ }
+ else
+ {
+ ScanKeyData skey[1];
+ SysScanDesc scan;
+
+ ScanKeyInit(&skey[0],
+ get_object_attnum_oid(object->classId),
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(object->objectId));
+
+    rel = table_open(object->classId, RowExclusiveLock);
+ scan = systable_beginscan(rel, get_object_oid_index(object->classId), true,
+  NULL, 1, skey);
+
+ /* we expect exactly one match */
+ tup = systable_getnext(scan);
+ if (HeapTupleIsValid(tup))
+    CatalogTupleDelete(rel, &tup->t_self);
+       else
+ elog(LOG, "could not find tuple for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+    systable_endscan(scan);
+    table_close(rel, RowExclusiveLock);
+ }
+}
+
 
regards,
Ranier Vilela

Re: Unify drop-by-OID functions

От
Ranier Vilela
Дата:
Only as a homework, is this function completely useless?

ItemPointerData
systable_scan_next(Relation heapRelation,
  Oid indexId,
  bool indexOK,
  Snapshot snapshot,
  int nkeys, ScanKey key)
{
    SysScanDesc scan;
HeapTuple htup;
ItemPointerData tid;

    scan = systable_beginscan(heapRelation, indexId, indexOK, snapshot, nkeys, key);
htup = systable_getnext(scan);
if (HeapTupleIsValid(htup))
        tid = &htup->t_self;
else
        tid = NULL;
    systable_endscan(scan);

return tid;
}

regards,
Ranier Vilela

Re: Unify drop-by-OID functions

От
Peter Eisentraut
Дата:
On 2020-05-04 20:57, Peter Eisentraut wrote:
> New patch attached.  I'll park it until PG14 opens.

committed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Unify drop-by-OID functions

От
Peter Eisentraut
Дата:
On 2020-05-05 18:05, Robert Haas wrote:
> This reminds me: I think that the issues in
> http://postgr.es/m/CA+TgmoYaFYRRdRZ94p_Qdt+1oONg6sMOvbkGHKVsFtONCrFkhw@mail.gmail.com
> should be considered here - we should guarantee that there's a
> snapshot registered continuously from before the call to
> SearchSysCache1 until after the call to CatalogTupleDelete. In the
> systable_beginscan case, we should be fine as long as the
> systable_endscan follows the CatalogTupleDelete call.

I considered this, but it seems independent of my patch.  If there are 
changes to be made, there are now fewer places to fix up.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services