autovacuum and reloptions

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On second look, however, the problem is that I seem to be
forced to declare all the autovacuum-related options and their parsing
properties in reloptions.c. This seems very ugly.

That was in fact the intended design, and is why the functions exist in
a separate file reloptions.c rather than having been dumped into some
existing place like heapam.c. I don't see anything very wrong with
having autovacuum options in StdRdOptions and default_reloptions().

There might at some point be a good case for inventing a plugin approach
here, but since autovacuum is a pretty much core component now, I don't
see the need to do so for it.

regards, tom lane

Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On second look, however, the problem is that I seem to be
forced to declare all the autovacuum-related options and their parsing
properties in reloptions.c. This seems very ugly.

That was in fact the intended design, and is why the functions exist in
a separate file reloptions.c rather than having been dumped into some
existing place like heapam.c. I don't see anything very wrong with
having autovacuum options in StdRdOptions and default_reloptions().

Hmm, OK. However, given that the various AMs amoptions also use
default_reloptions, they are going to accept the autovacuum options too.
Is that OK?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre@commandprompt.com> writes:

Hmm, OK. However, given that the various AMs amoptions also use
default_reloptions, they are going to accept the autovacuum options too.
Is that OK?

Well, we might want to split default_reloptions into two versions,
one for heaps and the other for indexes. But it doesn't bother me a
whole lot if we don't.

regards, tom lane

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Before we waste too much time thinking how this registering is to be
done, does anybody think that the current approach is OK and thus I
should just add the autovacuum options directly into StdRdOptions and
default_reloptions?

Given Simon's suggestion that i/o parameters should be per-tablespace I think
we might need to refactor this further.

I wonder if we could piggy-back on guc parameters. So you would register a guc
variable with a flag saying it's sensible to be set per-tablespace or
per-table.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's PostGIS support!

Gregory Stark <stark@enterprisedb.com> writes:

I wonder if we could piggy-back on guc parameters.

God, no. GUC is hopelessly complex already, we should *not* try to make
it track different values of a parameter for different tables.

Attaching a reloptions-like column to pg_tablespace might not be
unreasonable ... but I think that has little to do with Alvaro's
immediate problem.

regards, tom lane

Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

So I gave up waiting for someone else to do the reloptions patch for
autovacuum and started work on it myself.

Is it needed to keep backward compatibility?

I'd like to suggest to keep pg_catalog.pg_autovacuum as a system view
even after the options is put into reloptons, and the view to be
updatable using RULEs if possible.

Current pg_autovacuum-table approach has a benefit that
we can configure options by rule, for example:

INSERT INTO pg_autovacuu SELECT ... FROM pg_class WHERE ...;

But we will not able to do that if the settings will be in reloptions
because ALTER TABLE SET cannot be used with JOINs.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:

Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

So I gave up waiting for someone else to do the reloptions patch for
autovacuum and started work on it myself.

Is it needed to keep backward compatibility?

I'd like to suggest to keep pg_catalog.pg_autovacuum as a system view
even after the options is put into reloptons, and the view to be
updatable using RULEs if possible.

Ugh. No. It has been explicitly stated all along that pg_autovacuum
was a temporary API and that anyone depending on it could expect future
trouble.

But we will not able to do that if the settings will be in reloptions
because ALTER TABLE SET cannot be used with JOINs.

Any mechanism that a rule might use to set reloptions would be just
as usable in a join as the rule itself ...

regards, tom lane

Tom Lane wrote:

Gregory Stark <stark@enterprisedb.com> writes:

I wonder if we could piggy-back on guc parameters.

God, no. GUC is hopelessly complex already, we should *not* try to make
it track different values of a parameter for different tables.

Are there any more specific reasons than "it's very complex"? After
all, all the autovacuum options already exist as GUC parameters, so you
don't have to repeat all the validation code, for example.

Peter Eisentraut <peter_e@gmx.net> writes:

Tom Lane wrote:

God, no. GUC is hopelessly complex already, we should *not* try to make
it track different values of a parameter for different tables.

Are there any more specific reasons than "it's very complex"?

That one seems quite sufficient to me; but consider dump/restore if you
need more.

regards, tom lane

Alvaro Herrera escreveu:

So I gave up waiting for someone else to do the reloptions patch for
autovacuum and started work on it myself. What I soon discovered is
that on first blush it seems a lot easier than I had expected.

Sorry about that. :( I was swamped with PGCon Brasil and then I took
some days to rest. I'm expecting to finish it before next CF.

What did I already do? I refactored reloptions.c to support multiple
options. I tried to follow up the same way GUC do (of course, it is much
simpler). I'm thinking about removing (replacing?) StdRdOptions 'cause
we need a different struct to store reloptions. Suggestions?

I'm attaching the WIP patch so you can comment on it. I want to continue
working on it but I'm afraid you already did more than I do (in this
case, let me know for not duplicating efforts).

--
Euler Taveira de Oliveira
http://www.timbira.com/

Euler Taveira de Oliveira wrote:

What did I already do? I refactored reloptions.c to support multiple
options. I tried to follow up the same way GUC do (of course, it is much
simpler). I'm thinking about removing (replacing?) StdRdOptions 'cause
we need a different struct to store reloptions. Suggestions?

I'm attaching the WIP patch so you can comment on it. I want to continue
working on it but I'm afraid you already did more than I do (in this
case, let me know for not duplicating efforts).

Interesting. The main problem with this patch is that it loses the
ability to pass to parseRelOptions the set of options that are valid for
each context. Right now all callers use the same list comprising only
fillfactor, but my guess is that once we add new options it will make
sense to start differentiating. It makes no sense for indexes to have
autovacuum options, for example.

This is why I suggested in the email that started this thread we needed
some sort of registering capability (which was thrown down). I think
the main idea in your patch is fine, and better than what I was doing
which was just adding every option in default_reloptions. However it
needs to be adjusted somehow so that it doesn't mix all the options.
Maybe we could add a "category" bitmask for which each option would be
valid.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera escreveu:

Maybe we could add a "category" bitmask for which each option would be
valid.

The category tests are fine, that's why I introduced relopt_gen.kind but
I'm not using it yet. I'll try to refactor it to use bitmask (some
options could be used to both -- fillfactor) and to add it in the
validation code.

--
Euler Taveira de Oliveira
http://www.timbira.com/

Euler Taveira de Oliveira wrote:

Alvaro Herrera escreveu:

Maybe we could add a "category" bitmask for which each option would be
valid.

The category tests are fine, that's why I introduced relopt_gen.kind but
I'm not using it yet.

Ah, right, I hadn't noticed the kind stuff, maybe because it's unused
;-)

I'll try to refactor it to use bitmask (some options could be used to
both -- fillfactor) and to add it in the validation code.

Right, that's why I suggested using a bitmask.

Okay, so I'll let you deal with this for a while yet. A minor
suggestion: label the enum members distinctively, i.e. instead of
RO_BOOL perhaps use RELOPT_TYPE_BOOL, and RO_HEAP should be
RELOPT_KIND_HEAP (note this cannot be a plain enum, each category needs
a separate bit).

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre@commandprompt.com> writes:

Okay, so I'll let you deal with this for a while yet. A minor
suggestion: label the enum members distinctively, i.e. instead of
RO_BOOL perhaps use RELOPT_TYPE_BOOL, and RO_HEAP should be
RELOPT_KIND_HEAP (note this cannot be a plain enum, each category needs
a separate bit).

My first reaction was that the categories should just be the different
possible values of relkind. However, it also seems possible that there
could be index-AM-specific reloptions. So maybe what we need is a
categorization that is like relkind but breaks down RELKIND_INDEX into a
category per AM.

regards, tom lane

Euler Taveira de Oliveira wrote:

Alvaro Herrera escreveu:

So I gave up waiting for someone else to do the reloptions patch for
autovacuum and started work on it myself. What I soon discovered is
that on first blush it seems a lot easier than I had expected.

Sorry about that. :( I was swamped with PGCon Brasil and then I took
some days to rest. I'm expecting to finish it before next CF.

So did this go anywhere?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

I have a comment about reloptions of autovacuum parameters:
I'd like to have an easier way to extract individual parameters.

Alvaro Herrera <alvherre@commandprompt.com> wrote:

Euler Taveira de Oliveira wrote:

What did I already do? I refactored reloptions.c to support multiple
options. I tried to follow up the same way GUC do (of course, it is much
simpler). I'm thinking about removing (replacing?) StdRdOptions 'cause
we need a different struct to store reloptions. Suggestions?

Interesting.

We store reloptions as an array of 'key=value' text, but there is
no official way to read each parameter. I always create an user
defined function to extract fillfactors, but it would be better
if we have a standard method to do that.

---- [brute force way]
CREATE FUNCTION pg_fillfactor(reloptions text[], relam OID)
RETURNS integer AS
$$
SELECT (regexp_matches(array_to_string($1, '/'),
'fillfactor=([0-9]+)'))[1]::integer AS fillfactor
UNION ALL
SELECT CASE $2
WHEN 0 THEN 100 -- heap
WHEN 403 THEN 90 -- btree
WHEN 405 THEN 70 -- hash
WHEN 783 THEN 90 -- gist
WHEN 2742 THEN 100 -- gin
END
LIMIT 1;
$$
LANGUAGE sql STABLE;
----

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro escreveu:

[Sorry for the delay. I'm preparing the final patch and in a day or so
I'll post it.]

I have a comment about reloptions of autovacuum parameters:
I'd like to have an easier way to extract individual parameters.

Alvaro Herrera <alvherre@commandprompt.com> wrote:

Euler Taveira de Oliveira wrote:

What did I already do? I refactored reloptions.c to support multiple
options. I tried to follow up the same way GUC do (of course, it is much
simpler). I'm thinking about removing (replacing?) StdRdOptions 'cause
we need a different struct to store reloptions. Suggestions?

Interesting.

We store reloptions as an array of 'key=value' text, but there is
no official way to read each parameter. I always create an user
defined function to extract fillfactors, but it would be better
if we have a standard method to do that.

We will have an official function (getRelOptions()?) to extract the
reloption (autovacuum) parameters because we need to transform
pg_autovacuum catalog table in a view to maintain backward compatibility.

--
Euler Taveira de Oliveira
http://www.timbira.com/

Euler Taveira de Oliveira wrote:

We will have an official function (getRelOptions()?) to extract the
reloption (autovacuum) parameters because we need to transform
pg_autovacuum catalog table in a view to maintain backward compatibility.

Why? I'd say don't waste your time. If anything, it can be done after
the initial patch has been committed.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Euler Taveira de Oliveira <euler@timbira.com> wrote:

We will have an official function (getRelOptions()?) to extract the
reloption (autovacuum) parameters

Yeah, I want it, but...

because we need to transform
pg_autovacuum catalog table in a view to maintain backward compatibility.

I don't think we need pg_autovacuum in 8.4 because it is a deprecated API.
However, we should provide some easier methods to create an user-defined
pg_autovacuum view in case that users really need it.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center