Add comments

knizhnik · knizhnik · commit 4aaa7f9ab869 · 2017-02-01T18:29:35.000+03:00
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
@@ -1901,11 +1901,12 @@ FileWrite(File file, char *buffer, int amount)
 			inode = CFS_INODE(compressedSize, pos);
 			buffer = compressedBuffer;
 			amount = compressedSize;
+			/* cfs_encrypt will check if encryption is actually needed */
 			cfs_encrypt(buffer, VfdCache[file].seekPos, amount);
 		}
 		else
 		{
-			if (cfs_encryption)
+			if (cfs_encryption) /* we need to use buffer to perform encryption, so check if it is required */
 			{
 				memcpy(compressedBuffer, buffer, BLCKSZ);
 				buffer = compressedBuffer;
@@ -1946,12 +1947,6 @@ FileWrite(File file, char *buffer, int amount)
 		{
 			if (returnCode == amount)
 			{	
-                /* TODO. Is this comment still actual?
-				 * Verify that there is no race condition 
-				bool rc = pg_atomic_compare_exchange_u64((pg_atomic_uint64*)&VfdCache[file].map->inodes[VfdCache[file].seekPos / BLCKSZ],
-														 &prev_inode, inode);
-				Assert(rc);
-				*/
 				VfdCache[file].map->inodes[VfdCache[file].seekPos / BLCKSZ] = inode;
 				VfdCache[file].seekPos += BLCKSZ;
 				cfs_extend(VfdCache[file].map, VfdCache[file].seekPos);
diff --git a/src/include/storage/cfs.h b/src/include/storage/cfs.h
@@ -14,8 +14,15 @@
 #define CFS_DISABLE_TIMEOUT  1000   /* milliseconds */
 #define CFS_ESTIMATE_PROBES  10
 
-/* TODO Add comments */
+/* Maximal size of buffer for compressing (size) bytes where (size) is equal to PostgreSQL page size.
+ * Some compression algorithms requires to provide buffer large enough for worst case and immediately returns error is buffer is not enough.
+ * Accurate calculation of required buffer size is not needed here and doubling buffer size works for all used compression algorithms. */
 #define CFS_MAX_COMPRESSED_SIZE(size) ((size)*2)
+
+/* Minimal compression ratio when compression is expected to be reasonable.
+ * Right now it is hardcoded and equal to 2/3 of original size. If compressed image is larger than 2/3 of original image, 
+ * then uncompressed version of the page is stored.
+ */
 #define CFS_MIN_COMPRESSED_SIZE(size) ((size)*2/3)
 
 /* Possible options for compression algorithm choice */
@@ -40,7 +47,8 @@
 #define CFS_RC4_DROP_N		3072
 #define CFS_CIPHER_KEY_SIZE 256
 
-/* TODO Add comment */
+/* Inode type is 64 bit word storing offset and compressed size of the page. Size of Postgres segment is 1Gb, so using 32 bit offset is enough even through
+ * with compression size of compressed file can become larger than 1Gb if GC id disabled for long time */
 typedef uint64 inode_t;
 
 #define CFS_INODE_SIZE(inode) ((uint32)((inode) >> 32))
@@ -65,32 +73,41 @@ typedef struct
 	uint64 processedBytes;
 } CfsStatistic;
 
+/* CFS control state (maintained in shared memory) *.
 typedef struct
 {
-	/* TODO Add comment  */
+	/* Flag indicating that GC is in progress. It is used to prevent more than one GC. */
 	pg_atomic_flag gc_started;
 	/* Number of active garbage collection background workers. */
 	pg_atomic_uint32 n_active_gc;
 	/* Max number of garbage collection background workers.
 	 * Duplicates 'cfs_gc_workers' global variable. */
 	int            n_workers;
-	/* TODO Add comment */
+	/* Maximal number of iterations with GC should perform. Automatically started GC performs infinite number of iterations. 
+	 * Manually started GC performs just one iteration. */
 	int            max_iterations;
+	/* Flag for temporary didsabling GC */
 	bool           gc_enabled;
+	/* CFS GC statatistic */
 	CfsStatistic   gc_stat;
+	/* Encryption key */
 	uint8          cipher_key[CFS_CIPHER_KEY_SIZE];
 } CfsState;
 
+
+/* Map file format (mmap in memory and shared by all backends) */ 
 typedef struct
 {
-	/* TODO Add comment*/
+	/* Physical size of the file (size of the file on disk) */
 	pg_atomic_uint32 physSize;
-	/* TODO Add comment*/
+	/*  Virtual size of the file (Postgres page size (8k) * number of used pages) */
 	pg_atomic_uint32 virtSize;
-	/* TODO Add comment. What is the difference with physSize?*/
+	/* Total size of used pages. File may contain multiple versions of the same page, this is why physSize can be larger than usedSize */
 	pg_atomic_uint32 usedSize;
+	/* Lock used to synchronize access to the file */
 	pg_atomic_uint32 lock;
-	/* TODO Add comment */
+	/* PID (process identifier) of postmaster. We check it at open time to revoke lock in case when postgres is restarted. 
+	 * TODO: not so reliable because it can happen that occasionally postmaster will be given the same PID */
 	pid_t            postmasterPid;
 	/* Each pass of GC updates generation of the map */
 	uint64           generation;

Original file line number	Diff line number	Diff line change
`@@ -1901,11 +1901,12 @@ FileWrite(File file, char *buffer, int amount)`
`1901`	`1901`	`inode = CFS_INODE(compressedSize, pos);`
`1902`	`1902`	`buffer = compressedBuffer;`
`1903`	`1903`	`amount = compressedSize;`
	`1904`	`+ /* cfs_encrypt will check if encryption is actually needed */`
`1904`	`1905`	`cfs_encrypt(buffer, VfdCache[file].seekPos, amount);`
`1905`	`1906`	`}`
`1906`	`1907`	`else`
`1907`	`1908`	`{`
`1908`		`- if (cfs_encryption)`
	`1909`	`+ if (cfs_encryption) /* we need to use buffer to perform encryption, so check if it is required */`
`1909`	`1910`	`{`
`1910`	`1911`	`memcpy(compressedBuffer, buffer, BLCKSZ);`
`1911`	`1912`	`buffer = compressedBuffer;`
`@@ -1946,12 +1947,6 @@ FileWrite(File file, char *buffer, int amount)`
`1946`	`1947`	`{`
`1947`	`1948`	`if (returnCode == amount)`
`1948`	`1949`	`{`
`1949`		`- /* TODO. Is this comment still actual?`
`1950`		`- * Verify that there is no race condition`
`1951`		`- bool rc = pg_atomic_compare_exchange_u64((pg_atomic_uint64*)&VfdCache[file].map->inodes[VfdCache[file].seekPos / BLCKSZ],`
`1952`		`- &prev_inode, inode);`
`1953`		`- Assert(rc);`
`1954`		`- */`
`1955`	`1950`	`VfdCache[file].map->inodes[VfdCache[file].seekPos / BLCKSZ] = inode;`
`1956`	`1951`	`VfdCache[file].seekPos += BLCKSZ;`
`1957`	`1952`	`cfs_extend(VfdCache[file].map, VfdCache[file].seekPos);`