Re: [HACKERS] Custom compression methods

The most recent patch doesn't compile --without-lz4:

compress_lz4.c:191:17: error: ‘lz4_cmcheck’ undeclared here (not in a function)
  .datum_check = lz4_cmcheck,
...

And fails pg_upgrade check, apparently losing track of the compression (?)

 CREATE TABLE public.cmdata2 (
-    f1 text COMPRESSION lz4
+    f1 text
 );

You added pg_dump --no-compression, but the --help isn't updated.  I think
there should also be an option for pg_restore, like --no-tablespaces.  And I
think there should be a GUC for default_compression, like
default_table_access_method, so one can restore into an alternate compression
by setting PGOPTIONS=-cdefault_compression=lz4.

I'd like to be able to make all compressible columns of a table use a
non-default compression (except those which cannot), without having to use
\gexec...  We have tables with up to 1600 columns.  So a GUC would allow that.

Previously (on separate threads) I wondered whether pg_dump
--no-table-access-method was needed - maybe that be sufficient for this case,
too, but I think it should be possible to separately avoid restoring
compression AM and AM "proper".  So maybe it'd be like --no-tableam=compress
--no-tableam=storage or --no-tableam-all.

Some language fixes:

Subject: [PATCH v16 1/6] Built-in compression method

+++ b/doc/src/sgml/ddl.sgml
@@ -3762,6 +3762,8 @@ CREATE TABLE measurement (
        <productname>PostgreSQL</productname>
        tables (or, possibly, foreign tables).  It is possible to specify a
        tablespace and storage parameters for each partition separately.
+       Partitions inherits the compression method of the parent for each column
+       however we can set different compression method for each partition.

Should say:
+       By default, each column in a partition inherits the compression method from its parent table,
+       however a different compression method can be set for each partition.
 
+++ b/doc/src/sgml/ref/create_table.sgml
 
+       <varlistentry>
+        <term><literal>INCLUDING COMPRESSION</literal></term>
+        <listitem>
+         <para>
+          Compression method of the columns will be coppied.  The default
+          behavior is to exclude compression method, resulting in the copied
+          column will have the default compression method if the column type is
+          compressible.

Say:
+          Compression method of the columns will be copied.  The default
+          behavior is to exclude compression methods, resulting in the
+          columns having the default compression method.

+   <varlistentry>
+    <term><literal>COMPRESSION <replaceable class="parameter">compression_method</replaceable></literal></term>
+    <listitem>
+     <para>
+      This clause adds the compression method to a column.  Compression method
+      can be set from the available built-in compression methods.  The available
+      options are <literal>pglz</literal> and <literal>lz4</literal>.  If the
+      compression method is not sepcified for the compressible type then it will
+      have the default compression method.  The default compression method is
+      <literal>pglz</literal>.

Say "The compression method can be set from available compression methods" (or
remove this sentence).
Say "The available BUILT-IN methods are ..."
sepcified => specified

+
+	/*
+	 * No point in wasting a palloc cycle if value size is out of the allowed
+	 * range for compression

say "outside the allowed range"

+		if (pset.sversion >= 120000 &&
+               if (pset.sversion >= 120000 &&

A couple places that need to say >= 14

Subject: [PATCH v16 2/6] alter table set compression

+     <literal>SET COMPRESSION <replaceable class="parameter">compression_method</replaceable></literal>
+      This clause adds compression to a column. Compression method can be set
+      from available built-in compression methods. The available built-in
+      methods are <literal>pglz</literal> and <literal>lz4</literal>.

Should say "The compression method can be set to any available method.  The
built in methods are >PGLZ< or >LZ<"
That fixes grammar, and correction that it's possible to set to an available
method other than what's "built-in".

+++ b/src/include/commands/event_trigger.h
@@ -32,7 +32,7 @@ typedef struct EventTriggerData
 #define AT_REWRITE_ALTER_PERSISTENCE	0x01
 #define AT_REWRITE_DEFAULT_VAL			0x02
 #define AT_REWRITE_COLUMN_REWRITE		0x04
-
+#define AT_REWRITE_ALTER_COMPRESSION	0x08
 /*

This is losing a useful newline.

Subject: [PATCH v16 4/6] Create custom compression methods

+      This clause adds compression to a column. Compression method
+      could be created with <xref linkend="sql-create-access-method"/> or it can
+      be set from the available built-in compression methods.  The available
+      built-in methods are <literal>pglz</literal> and <literal>lz4</literal>.
+      The PRESERVE list contains list of compression methods used on the column
+      and determines which of them should be kept on the column.  Without
+      PRESERVE or if all the previous compression methods are not preserved then
+      the table will be rewritten.  If PRESERVE ALL is specified then all the
+      previous methods will be preserved and the table will not be rewritten.
      </para>
     </listitem>
    </varlistentry>

diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index f404dd1088..ade3989d75 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -999,11 +999,12 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
+      could be created with <xref linkend="sql-create-access-method"/> or it can
+      be set from the available built-in compression methods.  The available

remove this first "built-in" ?

+      built-in methods are <literal>pglz</literal> and <literal>lz4</literal>.


+GetCompressionAmRoutineByAmId(Oid amoid)
...
+	/* Check if it's an index access method as opposed to some other AM */
+	if (amform->amtype != AMTYPE_COMPRESSION)
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("access method \"%s\" is not of type %s",
+						NameStr(amform->amname), "INDEX")));
...
+				 errmsg("index access method \"%s\" does not have a handler",

In 3 places, the comment and code should say "COMPRESSION" right ?

Subject: [PATCH v16 6/6] Support compression methods options

+      If compression method has options they could be specified with
+      <literal>WITH</literal> parameter.

If *the* compression method has options, they *can* be specified with *the* ...

@@ -1004,7 +1004,9 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
+      method is <literal>pglz</literal>.  If the compression method has options
+      they could be specified by <literal>WITH</literal>
+      parameter.

same

+static void *
+lz4_cminitstate(List *options)
+{
+	int32	*acceleration = palloc(sizeof(int32));
+
+	/* initialize with the default acceleration */
+	*acceleration = 1;
+
+	if (list_length(options) > 0)
+	{
+		ListCell	*lc;
+
+		foreach(lc, options)
+		{
+			DefElem    *def = (DefElem *) lfirst(lc);
+
+			if (strcmp(def->defname, "acceleration") == 0)
+				*acceleration = pg_atoi(defGetString(def), sizeof(int32), 0);

Don't you need to say "else: error: unknown compression option" ?

+	/*
+	 * Compression option must be only valid if we are updating the compression
+	 * method.
+	 */
+	Assert(DatumGetPointer(acoptions) == NULL || OidIsValid(newcompression));
+

should say "need be valid only if .."

-- 
Justin

-- 
Justin