Control your disk usage in PG: Introduction to Disk Quota Extension

On Tue, 2018-11-13 at 16:47 +0800, Hubert Zhang wrote:

Hi all,

We implement disk quota feature on Postgresql as an extension(link:
https://github.com/greenplum-db/diskquota),
If you are interested, try and use it to limit the amount of disk
space that
a schema or a role can use in Postgresql.
Any feedback or question are appreciated.

It's not clear to me what's the goal of this thread? I understand what
quotas are about, but are you sharing it because (a) it's a useful
extension, (b) you propose adding a couple of new hooks (and keep the
extension separate) or (c) you propose adding both the hooks and the
extension (into contrib)?

I assume it's either (b) and (c), in which case you should add it to
2019-01 CF: https://commitfest.postgresql.org/21/

In either case, it might be useful to add a LICENSE to the github
repository, it's not clear what's the situation in this respect. That
probably matters especially for (b), because for (c) it'd end up with
PostgreSQL license automatically.

regards

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

Thanks, Tomas,

Yes, we want to add the hooks into postgres repo.
But before that, we plan to firstly get some feedbacks from community about
the diskquota extension implementation and usage?
Later, we'll modify our license and submit the hooks into CF.

Thanks
Hubert

On Wed, Nov 14, 2018 at 3:54 AM Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:

On Tue, 2018-11-13 at 16:47 +0800, Hubert Zhang wrote:

Hi all,

We implement disk quota feature on Postgresql as an extension(link:
https://github.com/greenplum-db/diskquota),
If you are interested, try and use it to limit the amount of disk
space that
a schema or a role can use in Postgresql.
Any feedback or question are appreciated.

It's not clear to me what's the goal of this thread? I understand what
quotas are about, but are you sharing it because (a) it's a useful
extension, (b) you propose adding a couple of new hooks (and keep the
extension separate) or (c) you propose adding both the hooks and the
extension (into contrib)?

I assume it's either (b) and (c), in which case you should add it to
2019-01 CF: https://commitfest.postgresql.org/21/

In either case, it might be useful to add a LICENSE to the github
repository, it's not clear what's the situation in this respect. That
probably matters especially for (b), because for (c) it'd end up with
PostgreSQL license automatically.

regards

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

--
Thanks

Hubert Zhang

Hi all,

We prepared a patch that includes the hook points. And such hook points are
needed for disk quota extension.
There are two hooks.
One is SmgrStat_hook. It's used to perform ad-hoc logic in storage when
doing smgr create/extend/truncate in general. As for disk quota extension,
this hook is used to detect active tables(new created tables, tables
extending new blocks, or tables being truncated)
The other is BufferExtendCheckPerms_hook. It's used to perform ad-hoc logic
when buffer extend a new block. Since ReadBufferExtended is a hot function,
we call this hook only when blockNum == P_NEW. As for disk quota
extension, this hook is used to do query enforcement during the query is
loading data.

Any comments are appreciated.

Regards,
Haozhou

On Wed, Nov 14, 2018 at 6:07 PM Hubert Zhang <hzhang@pivotal.io> wrote:

On Tue, Nov 20, 2018 at 2:20 AM Haozhou Wang <hawang@pivotal.io> wrote:

We prepared a patch that includes the hook points. And such hook points are needed for disk quota extension.
There are two hooks.
One is SmgrStat_hook. It's used to perform ad-hoc logic in storage when doing smgr create/extend/truncate in general. As for disk quota extension, this hook is used to detect active tables(new created tables, tables extending new blocks, or tables being truncated)
The other is BufferExtendCheckPerms_hook. It's used to perform ad-hoc logic when buffer extend a new block. Since ReadBufferExtended is a hot function, we call this hook only when blockNum == P_NEW. As for disk quota extension, this hook is used to do query enforcement during the query is loading data.

Any comments are appreciated.

+1 for adding some hooks to support this kind of thing, but I think
the names you've chosen are not very good. The hook name should
describe the place from which it is called, not the purpose for which
one imagines that it will be used, because somebody else might imagine
another use. Both BufferExtendCheckPerms_hook_type and
SmgrStat_hook_type are imagining that they know what the hook does -
CheckPerms in the first case and Stat in the second case.

For this particular purpose, I don't immediately see why you need a
hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

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

Thank you very much for your review.
We refactored our patch with new names and comments.

For ReadBufferExtended hook, yes, Readbuffer with P_NEW will then call
smgrextend.

But in smgrextend, we cannot get the oid of a relation, and it will take
some time to get the oid via smgrrelation.
We would like to add a hook just before the smgrextend to get the oid and
avoid use RelidByRelfilenode().

New patch is attached in the attachment.
Thank a lot!

Regards,
Haozhou

On Wed, Nov 21, 2018 at 10:48 PM Robert Haas <robertmhaas@gmail.com> wrote:

For this particular purpose, I don't immediately see why you need a
hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

For the usercase in diskquota.
BufferExtendCheckPerms_hook is used to do dynamic query enforcement, while
smgr related hooks are used to detect relfilenodeoid of active tables and
write them into shared memory(which is used to accelerate refreshing of
diskquota model).
The reason we don't use smgr_extend hook to replace ReadBuffer hook to do
enforcement has two folds:
1. As for enforcement, we don't want to affect the performance of insert
query. But hooks in smgr_extend need to convert relfilenode to reloid
firstly which need an indexscan.
2. Using hooks in ReadBuffer instead of smgr_extend could avoid to
enforcement on 'cluster relation' operator. For example, 'vacuum full
table' will firstly cluster and create a new table, and then delete the old
table. Because the disk usage will first grow and then shrink, if quota
limit is reached, then vacuum full will fail.(but in fact we want vacuum
full to reduce disk usage) Using hooks in ReadBuffer is one solution to
this problem. Of course, there are other solutions. But This is one of the
reason we use BufferExtendCheckPerms_hook to do enforcement at current
stage.

On Thu, Nov 22, 2018 at 7:26 PM Haozhou Wang <hawang@pivotal.io> wrote:

Thank you very much for your review.
We refactored our patch with new names and comments.

For ReadBufferExtended hook, yes, Readbuffer with P_NEW will then call
smgrextend.

But in smgrextend, we cannot get the oid of a relation, and it will take
some time to get the oid via smgrrelation.
We would like to add a hook just before the smgrextend to get the oid and
avoid use RelidByRelfilenode().

New patch is attached in the attachment.
Thank a lot!

Regards,
Haozhou

On Wed, Nov 21, 2018 at 10:48 PM Robert Haas <robertmhaas@gmail.com>
wrote:

On Tue, Nov 20, 2018 at 2:20 AM Haozhou Wang <hawang@pivotal.io> wrote:

We prepared a patch that includes the hook points. And such hook points

are needed for disk quota extension.

There are two hooks.
One is SmgrStat_hook. It's used to perform ad-hoc logic in storage when

doing smgr create/extend/truncate in general. As for disk quota extension,
this hook is used to detect active tables(new created tables, tables
extending new blocks, or tables being truncated)

The other is BufferExtendCheckPerms_hook. It's used to perform ad-hoc

logic when buffer extend a new block. Since ReadBufferExtended is a hot
function, we call this hook only when blockNum == P_NEW. As for disk quota
extension, this hook is used to do query enforcement during the query is
loading data.

Any comments are appreciated.

+1 for adding some hooks to support this kind of thing, but I think
the names you've chosen are not very good. The hook name should
describe the place from which it is called, not the purpose for which
one imagines that it will be used, because somebody else might imagine
another use. Both BufferExtendCheckPerms_hook_type and
SmgrStat_hook_type are imagining that they know what the hook does -
CheckPerms in the first case and Stat in the second case.

For this particular purpose, I don't immediately see why you need a
hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

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

--
Thanks

Hubert Zhang

On Wed, Nov 21, 2018 at 09:47:44AM -0500, Robert Haas wrote:

+1 for adding some hooks to support this kind of thing, but I think
the names you've chosen are not very good. The hook name should
describe the place from which it is called, not the purpose for which
one imagines that it will be used, because somebody else might imagine
another use. Both BufferExtendCheckPerms_hook_type and
SmgrStat_hook_type are imagining that they know what the hook does -
CheckPerms in the first case and Stat in the second case.

I personally don't mind making Postgres more pluggable, but I don't
think that we actually need the extra ones proposed here at the layer
of smgr, as smgr is already a layer designed to call an underlying set
of APIs able to extend, unlink, etc. depending on the storage type.

For this particular purpose, I don't immediately see why you need a
hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

Yes, that's a bit awkward.
--
Michael

Thanks very much for your comments.

To the best of my knowledge, smgr is a layer that abstract the storage
operations. Therefore, it is a good place to control or collect information
the storage operations without touching the physical storage layer.
Moreover, smgr is coming with actual disk IO operation (not consider the OS
cache) for postgres. So we do not need to worry about the buffer management
in postgres.
It will make the purpose of hook is pure: a hook for actual disk IO.

Regards,
Haozhou

On Wed, Dec 26, 2018 at 1:56 PM Michael Paquier <michael@paquier.xyz> wrote:

For this particular purpose, I don't immediately see why you need a
hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

Yes, that's a bit awkward.

Hi Michael, we revisit the ReadBuffer hook and remove it in the latest
patch.
ReadBuffer hook is original used to do enforcement(e.g. out of diskquota
limit) when query is loading data.
We plan to put the enforcement work of running query to separate diskquota
worker process.
Let worker process to detect the backends to be cancelled and send SIGINT
to these backends.
So there is no need for ReadBuffer hook anymore.

Our patch currently only contains smgr related hooks to catch the file
change and get the Active Table list for diskquota extension.

Thanks Hubert.

On Mon, Jan 7, 2019 at 6:56 PM Haozhou Wang <hawang@pivotal.io> wrote:

Thanks very much for your comments.

To the best of my knowledge, smgr is a layer that abstract the storage
operations. Therefore, it is a good place to control or collect information
the storage operations without touching the physical storage layer.
Moreover, smgr is coming with actual disk IO operation (not consider the
OS cache) for postgres. So we do not need to worry about the buffer
management in postgres.
It will make the purpose of hook is pure: a hook for actual disk IO.

Regards,
Haozhou

On Wed, Dec 26, 2018 at 1:56 PM Michael Paquier <michael@paquier.xyz>
wrote:

On Wed, Nov 21, 2018 at 09:47:44AM -0500, Robert Haas wrote:

+1 for adding some hooks to support this kind of thing, but I think
the names you've chosen are not very good. The hook name should
describe the place from which it is called, not the purpose for which
one imagines that it will be used, because somebody else might imagine
another use. Both BufferExtendCheckPerms_hook_type and
SmgrStat_hook_type are imagining that they know what the hook does -
CheckPerms in the first case and Stat in the second case.

I personally don't mind making Postgres more pluggable, but I don't
think that we actually need the extra ones proposed here at the layer
of smgr, as smgr is already a layer designed to call an underlying set
of APIs able to extend, unlink, etc. depending on the storage type.

For this particular purpose, I don't immediately see why you need a
hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

Yes, that's a bit awkward.
--
Michael

--
Thanks

Hubert Zhang

Hi Michael, Robert
For you question about the hook position, I want to explain more about the
background why we want to introduce these hooks.
We wrote a diskquota extension <https://github.com/greenplum-db/diskquota&gt;
for Postgresql(which is inspired by Heikki's pg_quota
<https://github.com/hlinnaka/pg_quota&gt;). Diskquota extension is used to
control the disk usage in Postgresql in a fine-grained way, which means:
1. You could set disk quota limit at schema level or role level.
2. A background worker will gather the current disk usage for each
schema/role in realtime.
3. A background worker will generate the blacklist for schema/role whose
quota limit is exceeded.
4. New transaction want to insert data into the schema/role in the
blacklist will be cancelled.

In step 2, gathering the current disk usage for each schema needs to sum
disk size of all the tables in this schema. This is a time consuming
operation. We want to use hooks in SMGR to detect the Active Table, and
only recalculate the disk size of all the Active Tables.
For example, the smgrextend hook indicates that you allocate a new block
and the table need to be treated as Active Table.

Do you have some better hook positions recommend to solve the above user
case?
Thanks in advance.

Hubert

On Tue, Jan 22, 2019 at 12:08 PM Hubert Zhang <hzhang@pivotal.io> wrote:

For this particular purpose, I don't immediately see why you need a

hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

Yes, that's a bit awkward.

Hi Michael, we revisit the ReadBuffer hook and remove it in the latest
patch.
ReadBuffer hook is original used to do enforcement(e.g. out of diskquota
limit) when query is loading data.
We plan to put the enforcement work of running query to separate diskquota
worker process.
Let worker process to detect the backends to be cancelled and send SIGINT
to these backends.
So there is no need for ReadBuffer hook anymore.

Our patch currently only contains smgr related hooks to catch the file
change and get the Active Table list for diskquota extension.

Thanks Hubert.

On Mon, Jan 7, 2019 at 6:56 PM Haozhou Wang <hawang@pivotal.io> wrote:

Thanks very much for your comments.

To the best of my knowledge, smgr is a layer that abstract the storage
operations. Therefore, it is a good place to control or collect information
the storage operations without touching the physical storage layer.
Moreover, smgr is coming with actual disk IO operation (not consider the
OS cache) for postgres. So we do not need to worry about the buffer
management in postgres.
It will make the purpose of hook is pure: a hook for actual disk IO.

Regards,
Haozhou

On Wed, Dec 26, 2018 at 1:56 PM Michael Paquier <michael@paquier.xyz>
wrote:

On Wed, Nov 21, 2018 at 09:47:44AM -0500, Robert Haas wrote:

+1 for adding some hooks to support this kind of thing, but I think
the names you've chosen are not very good. The hook name should
describe the place from which it is called, not the purpose for which
one imagines that it will be used, because somebody else might imagine
another use. Both BufferExtendCheckPerms_hook_type and
SmgrStat_hook_type are imagining that they know what the hook does -
CheckPerms in the first case and Stat in the second case.

I personally don't mind making Postgres more pluggable, but I don't
think that we actually need the extra ones proposed here at the layer
of smgr, as smgr is already a layer designed to call an underlying set
of APIs able to extend, unlink, etc. depending on the storage type.

For this particular purpose, I don't immediately see why you need a
hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

Yes, that's a bit awkward.
--
Michael

--
Thanks

Hubert Zhang

--
Thanks

Hubert Zhang

Hi all,

Currently we add hooks in SMGR to detect which table is being modified(disk
size change).
Inserting rows into existing page with room will not introduce new block,
and thus should not be treated as active table. smgrextend is a good
position to meet this behavior.
We welcome suggestions on other better hook positions!

Besides, suppose we use smgr as hook position, I want to discuss the API
passed to the hook function.
Take smgrextend as example. The function interface of smgrextend is like
that:
```
void smgrextend
(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, char *buffer,
bool skipFsync);
```
So the hook api of smgrextend could have two main options.
Hook API Option1
```
typedef void (*smgrextend_hook_type)
(RelFileNode smgr_rnode,ForkNumber forknum);
```
Hook API Option 2
```
typedef void (*smgrextend_hook_type)
(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,char *buffer,
bool skipFsync);
```

As for Option1. Since diskquota extension only needs the relfilenode
information to detect the active tables, Option1 just pass the RelFileNode
to the hook function. It's more clear and has semantic meaning.

While Option 2 is to pass the original parameters to the hook functions
without any filter. This is more general and let the different hook
implementations to decide how to use these parameters.

Option 1 also needs some additional work to handle smgrdounlinkall case,
whose input parameter is the SMgrRelation list. We may need to palloc
Relfilenode list and pfree it manually.
smgrdounlinkall function interface:
```
smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo, char
*relstorages)
```

Based on the assumption we use smgr as hook position, hook API option1 or
option2 which is better?
Or we could find some balanced API between option1 and option2?

Again comments on other better hook positions are appreciated!

Thanks
Hubert

On Wed, Jan 30, 2019 at 10:26 AM Hubert Zhang <hzhang@pivotal.io> wrote:

Hi Michael, Robert
For you question about the hook position, I want to explain more about the
background why we want to introduce these hooks.
We wrote a diskquota extension <https://github.com/greenplum-db/diskquota&gt;
for Postgresql(which is inspired by Heikki's pg_quota
<https://github.com/hlinnaka/pg_quota&gt;). Diskquota extension is used to
control the disk usage in Postgresql in a fine-grained way, which means:
1. You could set disk quota limit at schema level or role level.
2. A background worker will gather the current disk usage for each
schema/role in realtime.
3. A background worker will generate the blacklist for schema/role whose
quota limit is exceeded.
4. New transaction want to insert data into the schema/role in the
blacklist will be cancelled.

In step 2, gathering the current disk usage for each schema needs to sum
disk size of all the tables in this schema. This is a time consuming
operation. We want to use hooks in SMGR to detect the Active Table, and
only recalculate the disk size of all the Active Tables.
For example, the smgrextend hook indicates that you allocate a new block
and the table need to be treated as Active Table.

Do you have some better hook positions recommend to solve the above user
case?
Thanks in advance.

Hubert

On Tue, Jan 22, 2019 at 12:08 PM Hubert Zhang <hzhang@pivotal.io> wrote:

For this particular purpose, I don't immediately see why you need a

hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

Yes, that's a bit awkward.

Hi Michael, we revisit the ReadBuffer hook and remove it in the latest
patch.
ReadBuffer hook is original used to do enforcement(e.g. out of diskquota
limit) when query is loading data.
We plan to put the enforcement work of running query to separate
diskquota worker process.
Let worker process to detect the backends to be cancelled and send SIGINT
to these backends.
So there is no need for ReadBuffer hook anymore.

Our patch currently only contains smgr related hooks to catch the file
change and get the Active Table list for diskquota extension.

Thanks Hubert.

On Mon, Jan 7, 2019 at 6:56 PM Haozhou Wang <hawang@pivotal.io> wrote:

Thanks very much for your comments.

To the best of my knowledge, smgr is a layer that abstract the storage
operations. Therefore, it is a good place to control or collect information
the storage operations without touching the physical storage layer.
Moreover, smgr is coming with actual disk IO operation (not consider the
OS cache) for postgres. So we do not need to worry about the buffer
management in postgres.
It will make the purpose of hook is pure: a hook for actual disk IO.

Regards,
Haozhou

On Wed, Dec 26, 2018 at 1:56 PM Michael Paquier <michael@paquier.xyz>
wrote:

On Wed, Nov 21, 2018 at 09:47:44AM -0500, Robert Haas wrote:

+1 for adding some hooks to support this kind of thing, but I think
the names you've chosen are not very good. The hook name should
describe the place from which it is called, not the purpose for which
one imagines that it will be used, because somebody else might imagine
another use. Both BufferExtendCheckPerms_hook_type and
SmgrStat_hook_type are imagining that they know what the hook does -
CheckPerms in the first case and Stat in the second case.

I personally don't mind making Postgres more pluggable, but I don't
think that we actually need the extra ones proposed here at the layer
of smgr, as smgr is already a layer designed to call an underlying set
of APIs able to extend, unlink, etc. depending on the storage type.

For this particular purpose, I don't immediately see why you need a
hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

Yes, that's a bit awkward.
--
Michael

--
Thanks

Hubert Zhang

--
Thanks

Hubert Zhang

--
Thanks

Hubert Zhang

Hi,

On 2019-01-30 10:26:52 +0800, Hubert Zhang wrote:

Hi Michael, Robert
For you question about the hook position, I want to explain more about the
background why we want to introduce these hooks.
We wrote a diskquota extension <https://github.com/greenplum-db/diskquota&gt;
[ ...]
On Tue, Jan 22, 2019 at 12:08 PM Hubert Zhang <hzhang@pivotal.io> wrote:

For this particular purpose, I don't immediately see why you need a

hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

Yes, that's a bit awkward.

Please note that on PG lists the customary and desired style is to quote
inline in messages rather than top-quote.

Greetings,

Andres Freund

Hi Andres

On Sat, Feb 16, 2019 at 12:53 PM Andres Freund <andres@anarazel.de> wrote:

Hi,
On 2019-01-30 10:26:52 +0800, Hubert Zhang wrote:

Hi Michael, Robert
For you question about the hook position, I want to explain more about

the

background why we want to introduce these hooks.
We wrote a diskquota extension <

https://github.com/greenplum-db/diskquota&gt;

[ ...]
On Tue, Jan 22, 2019 at 12:08 PM Hubert Zhang <hzhang@pivotal.io> wrote:

For this particular purpose, I don't immediately see why you need a

hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

Yes, that's a bit awkward.

Please note that on PG lists the customary and desired style is to quote
inline in messages rather than top-quote.

Greetings,

Andres Freund

Thanks for your note. I will reply the above questions again.

On Wed, Nov 21, 2018 at 10:48 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Nov 20, 2018 at 2:20 AM Haozhou Wang <hawang@pivotal.io> wrote:

We prepared a patch that includes the hook points. And such hook points

are needed for disk quota extension.

There are two hooks.
One is SmgrStat_hook. It's used to perform ad-hoc logic in storage when

doing smgr create/extend/truncate in general. As for disk quota extension,
this hook is used to detect active tables(new created tables, tables
extending new blocks, or tables being truncated)

The other is BufferExtendCheckPerms_hook. It's used to perform ad-hoc

logic when buffer extend a new block. Since ReadBufferExtended is a hot
function, we call this hook only when blockNum == P_NEW. As for disk quota
extension, this hook is used to do query enforcement during the query is
loading data.

Any comments are appreciated.

+1 for adding some hooks to support this kind of thing, but I think
the names you've chosen are not very good. The hook name should
describe the place from which it is called, not the purpose for which
one imagines that it will be used, because somebody else might imagine
another use. Both BufferExtendCheckPerms_hook_type and
SmgrStat_hook_type are imagining that they know what the hook does -
CheckPerms in the first case and Stat in the second case.

For this particular purpose, I don't immediately see why you need a
hook in both places. If ReadBuffer is called with P_NEW, aren't we
guaranteed to end up in smgrextend()?

We have removed the hook in ReadBufferExtended and only keep the hooks in
SMGR.

On Wed, Dec 26, 2018 at 1:56 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Nov 21, 2018 at 09:47:44AM -0500, Robert Haas wrote:

+1 for adding some hooks to support this kind of thing, but I think
the names you've chosen are not very good. The hook name should
describe the place from which it is called, not the purpose for which
one imagines that it will be used, because somebody else might imagine
another use. Both BufferExtendCheckPerms_hook_type and
SmgrStat_hook_type are imagining that they know what the hook does -
CheckPerms in the first case and Stat in the second case.

I personally don't mind making Postgres more pluggable, but I don't
think that we actually need the extra ones proposed here at the layer
of smgr, as smgr is already a layer designed to call an underlying set
of APIs able to extend, unlink, etc. depending on the storage type.

The reason to use smgr as the hook position is as follows:
These hooks will be used by diskquota extension, which needs to gather the
current disk usage for each schema. We want to use hooks to detect the
Active Tables, and only recalculate the disk size of all the Active Tables.
As you mentioned, smgr is the layer to call underlying API to extend/unlink
files. So it's also the place to detect the table size change, i.e. the
Active Tables.
For example, the smgrextend hook indicates that you allocate a new block
and the corresponding table needs to be treated as Active Table.

Besides, suppose we use smgr as hook position, I want to discuss the API
passed to the hook function.
Take smgrextend as example. The function interface of smgrextend is like
that:
```
void smgrextend
(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, char *buffer,
bool skipFsync);
```
So the hook api of smgrextend could have two main options.
Hook API Option1
```
typedef void (*smgrextend_hook_type)
(RelFileNode smgr_rnode,ForkNumber forknum);
```
Hook API Option 2
```
typedef void (*smgrextend_hook_type)
(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,char *buffer,
bool skipFsync);
```

As for Option1. Since diskquota extension only needs the relfilenode
information to detect the active tables, Option1 just pass the RelFileNode
to the hook function. It's more clear and has semantic meaning.

While Option 2 is to pass the original parameters to the hook functions
without any filter. This is more general and let the different hook
implementations to decide how to use these parameters.

Option 1 also needs some additional work to handle smgrdounlinkall case,
whose input parameter is the SMgrRelation list. We may need to palloc
Relfilenode list and pfree it manually.
smgrdounlinkall function interface:
```
smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo, char
*relstorages)
```

Based on the assumption we use smgr as hook position, hook API option1 or
option2 which is better?
Or we could find some balanced API between option1 and option2?

Again comments on other better hook positions are also appreciated!

--
Thanks

Hubert Zhang

On Mon, Feb 18, 2019 at 7:39 PM Hubert Zhang <hzhang@pivotal.io> wrote:

Based on the assumption we use smgr as hook position, hook API option1 or option2 which is better?
Or we could find some balanced API between option1 and option2?

Again comments on other better hook positions are also appreciated!

Hi Hubert,

The July Commitfest is now running, and this entry is in "needs
review" state. Could you please post a rebased patch?

I have questions about how disk quotas should work and I think we'll
probably eventually want more hooks than these, but simply adding
these hooks so extensions can do whatever they want doesn't seem very
risky for core. I think it's highly likely that the hook signatures
will have to change in future releases too, but that seems OK for such
detailed internal hooks. As for your question, my first reaction was
that I preferred your option 1, because SMgrRelation seems quite
private and there are no existing examples of that object being
exposed to extensions. But on reflection, other callbacks don't take
such a mollycoddling approach. So my vote is for option 2 "just pass
all the arguments to the callback", which I understand to be the
approach of patch you have posted.

+    if (smgrcreate_hook)
+    {
+        (*smgrcreate_hook)(reln, forknum, isRedo);
+    }

Usually we don't use curlies for single line if branches.

--
Thomas Munro
https://enterprisedb.com

Thanks, Thomas.

On Mon, Jul 8, 2019 at 6:47 AM Thomas Munro <thomas.munro@gmail.com> wrote:

On Mon, Feb 18, 2019 at 7:39 PM Hubert Zhang <hzhang@pivotal.io> wrote:

Based on the assumption we use smgr as hook position, hook API option1

or option2 which is better?

Or we could find some balanced API between option1 and option2?

Again comments on other better hook positions are also appreciated!

Hi Hubert,

The July Commitfest is now running, and this entry is in "needs
review" state. Could you please post a rebased patch?

I have questions about how disk quotas should work and I think we'll
probably eventually want more hooks than these, but simply adding
these hooks so extensions can do whatever they want doesn't seem very
risky for core. I think it's highly likely that the hook signatures
will have to change in future releases too, but that seems OK for such
detailed internal hooks. As for your question, my first reaction was
that I preferred your option 1, because SMgrRelation seems quite
private and there are no existing examples of that object being
exposed to extensions. But on reflection, other callbacks don't take
such a mollycoddling approach. So my vote is for option 2 "just pass
all the arguments to the callback", which I understand to be the
approach of patch you have posted.

+    if (smgrcreate_hook)
+    {
+        (*smgrcreate_hook)(reln, forknum, isRedo);
+    }

Usually we don't use curlies for single line if branches.

I have rebased the patch to v4 and removed the unnecessary braces.
As your comments, Options 2 is still used in patch v4.

Agree that diskquota extension may use more hooks in future.
Currently the behavior of diskquota extension is that we use smgr hooks to
detect active tables and record them in the shared memory. Bgworkers of
diskquota extension will read these active tables from shared memory and
calculate the latest table size and sum them into the size of schema or
role. If size of schema of role exceeds their quota limit, they will be put
into a black list in shared memory. When a new query comes,
ExecutorCheckPerms_hook will be used to check the black list the cancel the
query if needed.

--
Thanks

Hubert Zhang

This patch no longer applies. Can you please rebase?

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

Thanks Alvaro!
I rebased this patch with the newest master branch. Attached the new
patch disk_quota_hooks_v5.patch in the attachment.

Regards,
Haozhou

On Thu, Sep 26, 2019 at 3:54 AM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

This patch no longer applies. Can you please rebase?

--
Álvaro Herrera
https://urldefense.proofpoint.com/v2/url?u=https-3A__www.2ndQuadrant.com_&amp;d=DwIDAw&amp;c=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw&amp;r=nGUCIcT5CVp6-pQcplYyagWnpAqoYm8YxWruds9UGI0&amp;m=aNzGoEI15bAE-vAivY34BtG2WdgrVojH-B-kjvuXsYA&amp;s=sT6zyiq4s8meelNuFw-lGD_mdvmUzv9zpVYWbFWusI0&amp;e=
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Regards,
Haozhou

On Fri, Sep 27, 2019 at 11:30:08AM +0800, Haozhou Wang wrote:

I rebased this patch with the newest master branch. Attached the new
patch disk_quota_hooks_v5.patch in the attachment.

This again needs a rebase, so I have switched it as waiting on
author.
--
Michael

Hi Michael,

Thank you very much for your email. I rebased the code with the newest
master and attached it in the attachment.

Thank you very much!

Regards,
Haozhou

On Sun, Dec 1, 2019 at 11:20 AM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Sep 27, 2019 at 11:30:08AM +0800, Haozhou Wang wrote:

I rebased this patch with the newest master branch. Attached the new
patch disk_quota_hooks_v5.patch in the attachment.

This again needs a rebase, so I have switched it as waiting on
author.
--
Michael

--
Regards,
Haozhou