Inefficient handling of LO-restore + Patch

I've detected that the restoring of large objects may consume huge amounts of
diskspace when using unusual blocksizes (e.g. 32KB). My setup is Postgresql-7.2.1
+ 32KB blocks + LOBLKSIZE 16KB, a unusual combination I think, , because this setup gave
the very best performance. I wanted to restore a database containing 2 gigabytes of
large objects, and noticed that it took around 6 gigabytes of diskspace to finish.
After it finished I ran "VACUUM FULL VERBOSE pg_largeobject",
and had around 140000 of live tuples, and around 480000 of dead tuples (I don't remember the exact
values, but I think there were 3 times dead tuples to live tuples).

I checked the pg_dump sources and found out that data is writen in 4KB chunks to the large object.
Since in my database the LO tuples are 16KB each, that would mean:
1. write 4KB -> have 1 live 4KB tuple
2. write 4KB -> 1 live 8KB tuple and 1 dead 4KB tuple
3. write 4KB -> 1 live 12KB tuple and 2 dead tuples
3. write 4KB -> 1 live 16KB tuple and 3 dead tuples

So creating a 16KB chunk took 16+12+8+4 => 40KB of diskspace, so recovering 2GB large objects
takes around 40/16 * 2 => 5GB diskspace and leaves 3 times the number of dead tuples (supposing
all LO's have sizes which are multples of 16KB).

I've written a patch which buffers LO's in 32KB blocks and tested again, and had 140000 live tuples
and nearly no dead tuples (around 10000, I'm still not sure where they're coming from).

Is there a better way to fix this? Can I post the patch to this list (~150 lines).And I did not find out how I can detect the large object chunksize, either from getting it from the headers (include "storage/large_object.h" did not work) or how to get it from the database I restore to. Any hints?

Best regards,
Mario Weilguni

"Mario Weilguni" <mario(dot)weilguni(at)icomedias(dot)com> writes:
> And I did not find out how I can detect the large object
> chunksize, either from getting it from the headers (include
> "storage/large_object.h" did not work)

Why not?

Still, it might make sense to move the LOBLKSIZE definition into
pg_config.h, since as you say it's of some interest to clients like
pg_dump.

regards, tom lane

Am Donnerstag, 11. April 2002 17:44 schrieb Tom Lane:
> "Mario Weilguni" <mario(dot)weilguni(at)icomedias(dot)com> writes:
> > And I did not find out how I can detect the large object
> > chunksize, either from getting it from the headers (include
> > "storage/large_object.h" did not work)
>

You did not answer if it's ok to post the patch, hope it's ok:
==================================
diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_archiver.c
postgresql-7.2.1/src/bin/pg_dump/pg_backup_archiver.c
--- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_archiver.c Mon Feb 11
01:18:20 2002
+++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_archiver.c Thu Apr 11 10:41:09
2002
@@ -819,6 +819,9 @@
AH->createdBlobXref = 1;
}

+ /* Initialize the LO Buffer */
+ AH->lo_buf_used = 0;
+
/*
* Start long-running TXs if necessary
*/
@@ -848,6 +851,19 @@
void
EndRestoreBlob(ArchiveHandle *AH, Oid oid)
{
+ if(AH->lo_buf_used > 0) {
+ /* Write remaining bytes from the LO buffer */
+ int res;
+ res = lo_write(AH->connection, AH->loFd, (void *) AH->lo_buf,
AH->lo_buf_used);
+
+ ahlog(AH, 5, "wrote remaining %d bytes of large object data (result =
%d)\n",
+ (int)AH->lo_buf_used, res);
+ if (res != AH->lo_buf_used)
+ die_horribly(AH, modulename, "could not write to large object (result: %d,
expected: %d)\n",
+ res, AH->lo_buf_used);
+ AH->lo_buf_used = 0;
+ }
+
lo_close(AH->connection, AH->loFd);
AH->writingBlob = 0;

@@ -1228,14 +1244,27 @@

if (AH->writingBlob)
{
- res = lo_write(AH->connection, AH->loFd, (void *) ptr, size * nmemb);
- ahlog(AH, 5, "wrote %d bytes of large object data (result = %d)\n",
- (int) (size * nmemb), res);
- if (res != size * nmemb)
+ if(AH->lo_buf_used + size * nmemb > AH->lo_buf_size) {
+ /* Split LO buffer */
+ int remaining = AH->lo_buf_size - AH->lo_buf_used;
+ int slack = nmemb * size - remaining;
+
+ memcpy(AH->lo_buf + AH->lo_buf_used, ptr, remaining);
+ res = lo_write(AH->connection, AH->loFd, AH->lo_buf, AH->lo_buf_size);
+ ahlog(AH, 5, "wrote %d bytes of large object data (result = %d)\n",
+ AH->lo_buf_size, res);
+ if (res != AH->lo_buf_size)
die_horribly(AH, modulename, "could not write to large object (result: %d,
expected: %d)\n",
- res, (int) (size * nmemb));
+ res, AH->lo_buf_size);
+ memcpy(AH->lo_buf, ptr + remaining, slack);
+ AH->lo_buf_used = slack;
+ } else {
+ /* LO Buffer is still large enough, buffer it */
+ memcpy(AH->lo_buf + AH->lo_buf_used, ptr, size * nmemb);
+ AH->lo_buf_used += size * nmemb;
+ }

- return res;
+ return size * nmemb;
}
else if (AH->gzOut)
{
diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_archiver.h
postgresql-7.2.1/src/bin/pg_dump/pg_backup_archiver.h
--- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_archiver.h Mon Nov 5
18:46:30 2001
+++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_archiver.h Thu Apr 11 10:41:14
2002
@@ -41,6 +41,7 @@
#include <errno.h>

#include "pqexpbuffer.h"
+#define LOBBUFSIZE 32768

#ifdef HAVE_LIBZ
#include <zlib.h>
@@ -240,6 +241,9 @@

RestoreOptions *ropt; /* Used to check restore options in
* ahwrite etc */
+ void *lo_buf;
+ int lo_buf_used;
+ int lo_buf_size;
} ArchiveHandle;

typedef struct _tocEntry
diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_custom.c
postgresql-7.2.1/src/bin/pg_dump/pg_backup_custom.c
--- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_custom.c Wed Nov 28
00:48:12 2001
+++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_custom.c Thu Apr 11 10:42:45
2002
@@ -153,6 +153,12 @@
if (ctx->zp == NULL)
die_horribly(AH, modulename, "out of memory\n");

+ /* Initialize LO buffering */
+ AH->lo_buf_size = LOBBUFSIZE;
+ AH->lo_buf = (void *)malloc(LOBBUFSIZE);
+ if(AH->lo_buf == NULL)
+ die_horribly(AH, modulename, "out of memory\n");
+
/*
* zlibOutSize is the buffer size we tell zlib it can output to. We
* actually allocate one extra byte because some routines want to
diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_files.c
postgresql-7.2.1/src/bin/pg_dump/pg_backup_files.c
--- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_files.c Thu Oct 25
07:49:52 2001
+++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_files.c Thu Apr 11 10:43:01
2002
@@ -113,6 +113,12 @@
AH->formatData = (void *) ctx;
ctx->filePos = 0;

+ /* Initialize LO buffering */
+ AH->lo_buf_size = LOBBUFSIZE;
+ AH->lo_buf = (void *)malloc(LOBBUFSIZE);
+ if(AH->lo_buf == NULL)
+ die_horribly(AH, modulename, "out of memory\n");
+
/*
* Now open the TOC file
*/
diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_null.c
postgresql-7.2.1/src/bin/pg_dump/pg_backup_null.c
--- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_null.c Wed Jun 27 23:21:37
2001
+++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_null.c Thu Apr 11 10:44:53 2002
@@ -64,7 +64,6 @@
*/
if (AH->mode == archModeRead)
die_horribly(AH, NULL, "this format cannot be read\n");
-
}

/*
diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_tar.c
postgresql-7.2.1/src/bin/pg_dump/pg_backup_tar.c
--- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_tar.c Sun Oct 28 07:25:58
2001
+++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_tar.c Thu Apr 11 10:44:08 2002
@@ -157,6 +157,12 @@
ctx = (lclContext *) malloc(sizeof(lclContext));
AH->formatData = (void *) ctx;
ctx->filePos = 0;
+
+ /* Initialize LO buffering */
+ AH->lo_buf_size = LOBBUFSIZE;
+ AH->lo_buf = (void *)malloc(LOBBUFSIZE);
+ if(AH->lo_buf == NULL)
+ die_horribly(AH, modulename, "out of memory\n");

/*
* Now open the TOC file
============================

>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Mario Weilguni wrote:
> Am Donnerstag, 11. April 2002 17:44 schrieb Tom Lane:
> > "Mario Weilguni" <mario(dot)weilguni(at)icomedias(dot)com> writes:
> > > And I did not find out how I can detect the large object
> > > chunksize, either from getting it from the headers (include
> > > "storage/large_object.h" did not work)
> >
>
> You did not answer if it's ok to post the patch, hope it's ok:
> ==================================
> diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_archiver.c
> postgresql-7.2.1/src/bin/pg_dump/pg_backup_archiver.c
> --- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_archiver.c Mon Feb 11
> 01:18:20 2002
> +++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_archiver.c Thu Apr 11 10:41:09
> 2002
> @@ -819,6 +819,9 @@
> AH->createdBlobXref = 1;
> }
>
> + /* Initialize the LO Buffer */
> + AH->lo_buf_used = 0;
> +
> /*
> * Start long-running TXs if necessary
> */
> @@ -848,6 +851,19 @@
> void
> EndRestoreBlob(ArchiveHandle *AH, Oid oid)
> {
> + if(AH->lo_buf_used > 0) {
> + /* Write remaining bytes from the LO buffer */
> + int res;
> + res = lo_write(AH->connection, AH->loFd, (void *) AH->lo_buf,
> AH->lo_buf_used);
> +
> + ahlog(AH, 5, "wrote remaining %d bytes of large object data (result =
> %d)\n",
> + (int)AH->lo_buf_used, res);
> + if (res != AH->lo_buf_used)
> + die_horribly(AH, modulename, "could not write to large object (result: %d,
> expected: %d)\n",
> + res, AH->lo_buf_used);
> + AH->lo_buf_used = 0;
> + }
> +
> lo_close(AH->connection, AH->loFd);
> AH->writingBlob = 0;
>
> @@ -1228,14 +1244,27 @@
>
> if (AH->writingBlob)
> {
> - res = lo_write(AH->connection, AH->loFd, (void *) ptr, size * nmemb);
> - ahlog(AH, 5, "wrote %d bytes of large object data (result = %d)\n",
> - (int) (size * nmemb), res);
> - if (res != size * nmemb)
> + if(AH->lo_buf_used + size * nmemb > AH->lo_buf_size) {
> + /* Split LO buffer */
> + int remaining = AH->lo_buf_size - AH->lo_buf_used;
> + int slack = nmemb * size - remaining;
> +
> + memcpy(AH->lo_buf + AH->lo_buf_used, ptr, remaining);
> + res = lo_write(AH->connection, AH->loFd, AH->lo_buf, AH->lo_buf_size);
> + ahlog(AH, 5, "wrote %d bytes of large object data (result = %d)\n",
> + AH->lo_buf_size, res);
> + if (res != AH->lo_buf_size)
> die_horribly(AH, modulename, "could not write to large object (result: %d,
> expected: %d)\n",
> - res, (int) (size * nmemb));
> + res, AH->lo_buf_size);
> + memcpy(AH->lo_buf, ptr + remaining, slack);
> + AH->lo_buf_used = slack;
> + } else {
> + /* LO Buffer is still large enough, buffer it */
> + memcpy(AH->lo_buf + AH->lo_buf_used, ptr, size * nmemb);
> + AH->lo_buf_used += size * nmemb;
> + }
>
> - return res;
> + return size * nmemb;
> }
> else if (AH->gzOut)
> {
> diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_archiver.h
> postgresql-7.2.1/src/bin/pg_dump/pg_backup_archiver.h
> --- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_archiver.h Mon Nov 5
> 18:46:30 2001
> +++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_archiver.h Thu Apr 11 10:41:14
> 2002
> @@ -41,6 +41,7 @@
> #include <errno.h>
>
> #include "pqexpbuffer.h"
> +#define LOBBUFSIZE 32768
>
> #ifdef HAVE_LIBZ
> #include <zlib.h>
> @@ -240,6 +241,9 @@
>
> RestoreOptions *ropt; /* Used to check restore options in
> * ahwrite etc */
> + void *lo_buf;
> + int lo_buf_used;
> + int lo_buf_size;
> } ArchiveHandle;
>
> typedef struct _tocEntry
> diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_custom.c
> postgresql-7.2.1/src/bin/pg_dump/pg_backup_custom.c
> --- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_custom.c Wed Nov 28
> 00:48:12 2001
> +++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_custom.c Thu Apr 11 10:42:45
> 2002
> @@ -153,6 +153,12 @@
> if (ctx->zp == NULL)
> die_horribly(AH, modulename, "out of memory\n");
>
> + /* Initialize LO buffering */
> + AH->lo_buf_size = LOBBUFSIZE;
> + AH->lo_buf = (void *)malloc(LOBBUFSIZE);
> + if(AH->lo_buf == NULL)
> + die_horribly(AH, modulename, "out of memory\n");
> +
> /*
> * zlibOutSize is the buffer size we tell zlib it can output to. We
> * actually allocate one extra byte because some routines want to
> diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_files.c
> postgresql-7.2.1/src/bin/pg_dump/pg_backup_files.c
> --- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_files.c Thu Oct 25
> 07:49:52 2001
> +++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_files.c Thu Apr 11 10:43:01
> 2002
> @@ -113,6 +113,12 @@
> AH->formatData = (void *) ctx;
> ctx->filePos = 0;
>
> + /* Initialize LO buffering */
> + AH->lo_buf_size = LOBBUFSIZE;
> + AH->lo_buf = (void *)malloc(LOBBUFSIZE);
> + if(AH->lo_buf == NULL)
> + die_horribly(AH, modulename, "out of memory\n");
> +
> /*
> * Now open the TOC file
> */
> diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_null.c
> postgresql-7.2.1/src/bin/pg_dump/pg_backup_null.c
> --- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_null.c Wed Jun 27 23:21:37
> 2001
> +++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_null.c Thu Apr 11 10:44:53 2002
> @@ -64,7 +64,6 @@
> */
> if (AH->mode == archModeRead)
> die_horribly(AH, NULL, "this format cannot be read\n");
> -
> }
>
> /*
> diff -Nur postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_tar.c
> postgresql-7.2.1/src/bin/pg_dump/pg_backup_tar.c
> --- postgresql-7.2.1-orig/src/bin/pg_dump/pg_backup_tar.c Sun Oct 28 07:25:58
> 2001
> +++ postgresql-7.2.1/src/bin/pg_dump/pg_backup_tar.c Thu Apr 11 10:44:08 2002
> @@ -157,6 +157,12 @@
> ctx = (lclContext *) malloc(sizeof(lclContext));
> AH->formatData = (void *) ctx;
> ctx->filePos = 0;
> +
> + /* Initialize LO buffering */
> + AH->lo_buf_size = LOBBUFSIZE;
> + AH->lo_buf = (void *)malloc(LOBBUFSIZE);
> + if(AH->lo_buf == NULL)
> + die_horribly(AH, modulename, "out of memory\n");
>
> /*
> * Now open the TOC file
> ============================
>
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 4: Don't 'kill -9' the postmaster
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Patch applied. Thanks.

---------------------------------------------------------------------------

Mario Weilguni wrote:
> Am Donnerstag, 11. April 2002 17:44 schrieb Tom Lane:
> > "Mario Weilguni" <mario(dot)weilguni(at)icomedias(dot)com> writes:
> > > And I did not find out how I can detect the large object
> > > chunksize, either from getting it from the headers (include
> > > "storage/large_object.h" did not work)
> >
>
> You did not answer if it's ok to post the patch, hope it's ok:

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

This patch does not compile correctly:

pg_backup_archiver.c: In function `ahwrite':
pg_backup_archiver.c:1252: warning: pointer of type `void *' used in arithmetic
pg_backup_archiver.c:1259: warning: pointer of type `void *' used in arithmetic
pg_backup_archiver.c:1263: warning: pointer of type `void *' used in arithmetic
make: *** [pg_backup_archiver.o] Error 1

Bruce Momjian writes:

>
> Patch applied. Thanks.
>
> ---------------------------------------------------------------------------
>
>
> Mario Weilguni wrote:
> > Am Donnerstag, 11. April 2002 17:44 schrieb Tom Lane:
> > > "Mario Weilguni" <mario(dot)weilguni(at)icomedias(dot)com> writes:
> > > > And I did not find out how I can detect the large object
> > > > chunksize, either from getting it from the headers (include
> > > > "storage/large_object.h" did not work)
> > >
> >
> > You did not answer if it's ok to post the patch, hope it's ok:
>
>

--
Peter Eisentraut peter_e(at)gmx(dot)net

OK, I have applied the following patch to fix these warnings. However,
I need Mario to confirm these are the right changes. Thanks.

---------------------------------------------------------------------------

Peter Eisentraut wrote:
> This patch does not compile correctly:
>
> pg_backup_archiver.c: In function `ahwrite':
> pg_backup_archiver.c:1252: warning: pointer of type `void *' used in arithmetic
> pg_backup_archiver.c:1259: warning: pointer of type `void *' used in arithmetic
> pg_backup_archiver.c:1263: warning: pointer of type `void *' used in arithmetic
> make: *** [pg_backup_archiver.o] Error 1
>
>
> Bruce Momjian writes:
>
> >
> > Patch applied. Thanks.
> >
> > ---------------------------------------------------------------------------
> >
> >
> > Mario Weilguni wrote:
> > > Am Donnerstag, 11. April 2002 17:44 schrieb Tom Lane:
> > > > "Mario Weilguni" <mario(dot)weilguni(at)icomedias(dot)com> writes:
> > > > > And I did not find out how I can detect the large object
> > > > > chunksize, either from getting it from the headers (include
> > > > > "storage/large_object.h" did not work)
> > > >
> > >
> > > You did not answer if it's ok to post the patch, hope it's ok:
> >
> >
>
> --
> Peter Eisentraut peter_e(at)gmx(dot)net
>
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Am Mittwoch, 24. April 2002 16:03 schrieb Bruce Momjian:
> OK, I have applied the following patch to fix these warnings. However,
> I need Mario to confirm these are the right changes. Thanks.

I've checked it and works fine, but the memcpy() prototype says it should be
void pointers. Will this give errors with non-gcc compilers?

Mario Weilguni wrote:
> Am Mittwoch, 24. April 2002 16:03 schrieb Bruce Momjian:
> > OK, I have applied the following patch to fix these warnings. However,
> > I need Mario to confirm these are the right changes. Thanks.
>
> I've checked it and works fine, but the memcpy() prototype says it should be
> void pointers. Will this give errors with non-gcc compilers?

No, it is fine. Anything can be cast _to_ a void pointer. You just
can't do arithmetic on them.

Are you sure you want to use 'void *' in your code. Looking at the
backend large object code, I see char *:

extern int inv_read(LargeObjectDesc *obj_desc, char *buf, int nbytes);
extern int inv_write(LargeObjectDesc *obj_desc, char *buf, int nbytes);

I guess my point is that these are just streams of bytes, _not_ really
streams of items of unknown length. We know the length, and the length
is char. This may simplify the code.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026