Re: [Patch] Space reservation (pgupgrade)

Heikki Linnakangas napsal(a):
> 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.

No, it is preupgrade script for 8.4->8.5, better is say it is code which try to 
make space on page - part of future preupgrade script. It will be useful when 
8.5 will be out. But probably we will need to make some improvements in 8.4 code.

> 
>> 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.

Yes, it is reason why I use int instead of bool, which should be use differently 
in each version. Of course nobody know what will happen in the future 
development, but I currently implement what I know that we need. If we will need 
more than ...

> 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.

The problem is with CREATE TABLE/INDEX command. If somebody creates table then 
table will not have correctly set attributes and new table is not prepared for 
upgrade. There are of course more solution like forbidden create table/index 
command during preupgrade or set reserved space for relation based on already 
know constants. I prefer now easiest way and it is to have three statuses.

The idea of preupgrade script is following:

1) for each relation calculate reserved space per page and per tuple. Per page 
is just difference between page headers size and maybe special size. But per 
tuple it requires to calculate per attribute difference - potential null values 
will be ignored. The calculation will be know when next release, but we need 
basic support for it.

2) when reserved space is set then preupgrade script start to process all 
relation and perform appropriate operation on the block.

4) upgrade check in single mode if all relations have correct status.

>> 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?

As I mention several times, data type change should be handled differently. It 
should invoke to create new datatype and keep old datatype implementation (for 
example in separate legacy library).

Tuple size difference is calculate on following structures:

HeapTupleHeader (including DatumTupleHeader for composite datatypes)
OID size (+Security tag)
Array structures
Varlena implementation
Toast pointer

preupgrade script should know difference in size in these structures and takes 
all attributes and count maximal additional size.

> 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.

Yes but you need to have infrastructure. Better and safer is backport just a 
constants then big amount of code. Probably we will be sometime in situation 
when we will need to backport more but every time we can postpone problematic 
feature and update inplace upgrade infrastructure in current development 
release. See CRC example.

> 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.

As I mention, It is what i want. Create basic infrastructure and constants will 
be set and backported when 8.5 will be out.
    thanks Zdenek