Re: [Patch] Space reservation (pgupgrade)

Zdenek Kotala wrote:
> I attached patch which add capability to reserve space on page for
> future upgrade. It is mandatory for future in-place upgrade 
> implementation. This patch contains basic infrastructure not preupgrade 
> script itself. I'm going to send WIP preupgrade script today in separate 
> mail.

Is that a preupgrade script for upgrading from 8.3 to 8.4? As such, it 
can't take advantage of any of the changes in this patch.

> This patch contains following modifications:
> 
> 1) I added datpreupgstatus and relpreupgstatus attribute into 
> pg_database and pg_class. Original idea was to use only flag, but I need 
> more info for tracking several process status (like 0 - not set, 1 - 
> reserved space set, 2 - reservation is finished and so on).
> 
> I'm not sure if datpreupgstatus will be useful, but I think better is to 
> have it here.

I think this is too flexible and not flexible enough at the same time. 
It's too flexible, because we don't know what information we'd need to 
store about relations in a future script. Those fields might go unused 
for many releases, just confusing people. It's not flexible enough, if 
it turns out that we need to store more information about relations.

Predicting features that have not yet been written is hard, isn't it. 
Trying to do it too precisely will just lead to failure. The generic 
approach of using a pre-upgrade script is good, but I don't think it's 
wise to prepare anything more specific than that.

It seems that those flags were meant to keep track of what databases and 
relations have already been processed by the pre-upgrade script. I don't 
think the script needs to be restartable. If we accept that the whole 
cluster must be processed in one go, we don't need so much bookkeeping. 
Remember that this is a tool that we'll need to backport to people's 
production systems, so better keep it as simple as possible.

> 2) I added two reloption rs_perpage and rs_pertuple for setup amount of 
> reserved space. I think these two attributes are enough for configure 
> all case. Keep in mind that for each relation could be these parameters 
> different.

I'm afraid these too are too flexible and not flexible enough.

For example, if we change the representation of a data type so that some 
values become longer, some shorter, how much space would you reserve per 
page and per tuple?

In the future release, when we know exactly what the new on-disk format 
looks like, we can backpatch a patch that reserves the right amount of 
space on pages.

Note that from a testing point of view, those reloptions would go unused 
until it's time to upgrade to the next release, so it wouldn't be 
significantly less risky to just backpatch code to do the calculation at 
that point, vs. implementing some generic formula based on per-page and 
per-tuple reservation earlier.

> 3) I adapted source code to respect new reloptions. Basic idea of it is 
> that before someone call PageAddItem it checks free space on a page 
> (PageGetFreeSpace...). I modify PageGetFreeSpace function to count 
> reserved space. Unfortunately, it requires additional parameters.

Yeah, I think that's the right place to do it.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com